From 97951027dc06ef419383925e86bd6f2143dfd51d Mon Sep 17 00:00:00 2001 From: Sven Gothel Date: Fri, 18 Sep 2020 05:51:22 +0200 Subject: DBTDevice: Resolve disconnect/remove resource race condition @ rapid connect w/ one thread per device Testing w/ 2 devices in fastest rapid reconnect (using automatic disconnect) on Raspberry-Pi: (1) Native run-dbt_scanner10.sh -silent_gatt -mac C0:26:DA:01:DA:B1 -mac C0:26:DF:01:E5:CA -disconnect -count 12 (2) Java run-java-scanner10.sh -silent_gatt -mac C0:26:DA:01:DA:B1 -mac C0:26:DF:01:E5:CA -disconnect -count 12 The Java (2) test case disclosed a race condition as follows: "An application using one thread per device and rapid connect, should either use disconnect() or remove(), but never issue remove() after disconnect(). Doing so would eventually delete the device being already in use by another thread due to discovery post disconnect!" The example code was issuing (1) disconnect and waiting for being diconnected and only then (2) removing the device. In between the device got discovered already and a new processing thread has started while the closing previous processing thread is removing the device and hence the underlying DBTDevice. A user shall either use disconnect() or remove(), period. --- api/direct_bt/DBTDevice.hpp | 16 +++++++++- examples/direct_bt_scanner10/dbt_scanner10.cpp | 9 +++--- examples/java/ScannerTinyB10.java | 12 ++------ java/direct_bt/tinyb/DBTDevice.java | 5 +--- java/org/tinyb/BluetoothDevice.java | 41 +++++++++++++++++++++----- 5 files changed, 57 insertions(+), 26 deletions(-) diff --git a/api/direct_bt/DBTDevice.hpp b/api/direct_bt/DBTDevice.hpp index a6a5c46e..0f0cb695 100644 --- a/api/direct_bt/DBTDevice.hpp +++ b/api/direct_bt/DBTDevice.hpp @@ -306,7 +306,13 @@ namespace direct_bt { * when AdapterStatusListener::deviceDisconnected(..) has been received. *

*

- * An open GATTHandler will also be closed. + * An open GATTHandler will also be closed.
+ * The connection to this device is removed, removing all connected profiles. + *

+ *

+ * An application using one thread per device and rapid connect, should either use disconnect() or remove(), + * but never issue remove() after disconnect(). Doing so would eventually delete the device being already + * in use by another thread due to discovery post disconnect! *

* @return HCIStatusCode::SUCCESS if the command has been accepted, otherwise HCIStatusCode may disclose reason for rejection. */ @@ -329,6 +335,14 @@ namespace direct_bt { *

* This method is automatically called @ destructor. *

+ *

+ * This method is an atomic operation. + *

+ *

+ * An application using one thread per device and rapid connect, should either use disconnect() or remove(), + * but never issue remove() after disconnect(). Doing so would eventually delete the device being already + * in use by another thread due to discovery post disconnect! + *

*/ void remove(); diff --git a/examples/direct_bt_scanner10/dbt_scanner10.cpp b/examples/direct_bt_scanner10/dbt_scanner10.cpp index 84d2ff13..f1f296fa 100644 --- a/examples/direct_bt_scanner10/dbt_scanner10.cpp +++ b/examples/direct_bt_scanner10/dbt_scanner10.cpp @@ -368,15 +368,14 @@ exit: fprintf(stderr, "****** Processing Device: pingGATT failed: %s\n", device->getAddressString().c_str()); } - fprintf(stderr, "****** Processing Device: disconnecting: %s\n", device->getAddressString().c_str()); - device->disconnect(); // will implicitly purge the GATT data, including GATTCharacteristic listener. - while( device->getConnected() ) { - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - } if( REMOVE_DEVICE ) { fprintf(stderr, "****** Processing Device: removing: %s\n", device->getAddressString().c_str()); device->remove(); + } else { + fprintf(stderr, "****** Processing Device: disconnecting: %s\n", device->getAddressString().c_str()); + device->disconnect(); // will implicitly purge the GATT data, including GATTCharacteristic listener. } + if( !SILENT_GATT ) { device->getAdapter().printSharedPtrListOfDevices(); } diff --git a/examples/java/ScannerTinyB10.java b/examples/java/ScannerTinyB10.java index 3afb97e4..d40864af 100644 --- a/examples/java/ScannerTinyB10.java +++ b/examples/java/ScannerTinyB10.java @@ -384,18 +384,12 @@ public class ScannerTinyB10 { println("****** Processing Device: pingGATT failed: "+device.getAddress()); } - println("****** Processing Device: disconnecting: "+device.getAddress()); - device.disconnect(); // will implicitly purge the GATT data, including GATTCharacteristic listener. - while( device.getConnected() ) { - try { - Thread.sleep(100); - } catch (final InterruptedException e) { - e.printStackTrace(); - } - } if( REMOVE_DEVICE ) { println("****** Processing Device: removing: "+device.getAddress()); device.remove(); + } else { + println("****** Processing Device: disconnecting: "+device.getAddress()); + device.disconnect(); // will implicitly purge the GATT data, including GATTCharacteristic listener. } if( 0 < MULTI_MEASUREMENTS ) { diff --git a/java/direct_bt/tinyb/DBTDevice.java b/java/direct_bt/tinyb/DBTDevice.java index 30c5bb6a..ae361ffc 100644 --- a/java/direct_bt/tinyb/DBTDevice.java +++ b/java/direct_bt/tinyb/DBTDevice.java @@ -238,9 +238,6 @@ public class DBTDevice extends DBTObject implements BluetoothDevice if( !isValid() ) { return; } - // GATTHandler::removeAllCharacteristicListener(): implicit via device.disconnect -> gatt.disconnect - disconnectImpl(); // make sure, regardless of isConnected state - disableConnectedNotifications(); disableRSSINotifications(); disableManufacturerDataNotifications(); @@ -586,7 +583,7 @@ public class DBTDevice extends DBTObject implements BluetoothDevice */ @Override public final boolean remove() throws BluetoothException { - // close: disconnectImpl(), clear java-listener, super.close() + // close: clear java-listener, super.close() // -> DBTNativeDownlink.delete(): deleteNativeJavaObject(..), deleteImpl(..) -> DBTDevice::remove() close(); // return removeImpl(); diff --git a/java/org/tinyb/BluetoothDevice.java b/java/org/tinyb/BluetoothDevice.java index 5cc8ff63..c4bc99ec 100644 --- a/java/org/tinyb/BluetoothDevice.java +++ b/java/org/tinyb/BluetoothDevice.java @@ -81,11 +81,13 @@ public interface BluetoothDevice extends BluetoothObject * when {@link AdapterStatusListener#deviceDisconnected(BluetoothDevice, HCIStatusCode, long)} has been received. *

*

- * Any open GATT connection will be closed as well. + * An open GATT connection will also be closed.
+ * The connection to this device is removed, removing all connected profiles. *

*

- * tinyb.dbus The connection to this device is removed, removing all connected - * profiles. + * An application using one thread per device and rapid connect, should either use {@link #disconnect()} or {@link #remove()}, + * but never issue remove() after disconnect(). Doing so would eventually delete the device being already + * in use by another thread due to discovery post disconnect! *

* @return {@link HCIStatusCode#SUCCESS} if the command has been accepted, otherwise {@link HCIStatusCode} may disclose reason for rejection. * @since 2.1.0 change API, i.e. return value from boolean to HCIStatusCode in favor of direct_bt @@ -181,10 +183,35 @@ public interface BluetoothDevice extends BluetoothObject */ boolean pair() throws BluetoothException; - /** Remove this device from the system (like an unpair). - * @return TRUE if the device has been removed - * @throws BluetoothException - */ + /** + * Remove this device from the system (like an unpair). + *

+ * Direct-BT: Disconnects this device via disconnect(..) and + * explicitly removes its shared references from the Adapter: + * connected-devices, discovered-devices and shared-devices. + *

+ *

+ * This method shall be issued to ensure no device reference will + * be leaked in a long lived adapter, + * as only its reference within connected-devices and discovered-devices are removed at disconnect. + *

+ *

+ * After calling this method, the device shall no more being used. + *

+ *

+ * This method is automatically called @ destructor. + *

+ *

+ * This method is an atomic operation. + *

+ *

+ * An application using one thread per device and rapid connect, should either use {@link #disconnect()} or {@link #remove()}, + * but never issue remove() after disconnect(). Doing so would eventually delete the device being already + * in use by another thread due to discovery post disconnect! + *

+ * @return TRUE if the device has been removed + * @throws BluetoothException + */ boolean remove() throws BluetoothException; /** Cancels an initiated pairing operation -- cgit v1.2.3