DBTAdapter: Split mtx_deviceReferences for each list; Avoid callback-scope locks, leading to deadlocks eventually

Split mtx_deviceReferences into one mtx per list discoveredDevices, connectedDevices and sharedDevices,
allowing us to lock only access to the actual list in a most atomic operation.

+++

Avoid callback-scope locks, leading to deadlocks eventually

A global device-ref mutex (as was mtx_deviceReferences) across the whole callback
method eventually will lead to a deadlock.
This especially as most DBTManager or HCIHandler low-level callbacks
call into all AdapterStatusListener listener.
They would just need to perform one off-thread operation
and access the already locked device-ref mutex to deadlock.
This happened while testing adapter power-off and -on
with running devices.

+++

Reducing the deadlock-surface by using per resource locking
and only at the minimal scope possible avoids this dilemma.

All access method (find, add, remove) already lock their resources with this change.
Since we utilize shared_ptr, after fetching a device it is safe from destruction.

Hence added emphasis on the mtx_sharedDevices,
as the sharedDevices list is the final holder of DBTDevice lifecycle.

In removeDevice(..) and adapter's dtor, we hence lock the mtx_sharedDevice.

1 files changed, 30 insertions, 29 deletions

diff --git a/src/direct_bt/DBTAdapter.cpp b/src/direct_bt/DBTAdapter.cpp
index 455ba655..abb925c9 100644
--- a/src/direct_bt/DBTAdapter.cpp
+++ b/src/direct_bt/DBTAdapter.cpp
@@ -85,7 +85,7 @@ std::shared_ptr<DBTDevice> DBTAdapter::findDevice(std::vector<std::shared_ptr<DB
 }
 
 bool DBTAdapter::addConnectedDevice(const std::shared_ptr<DBTDevice> & device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_connectedDevices); // RAII-style acquire and relinquish via destructor
     if( nullptr != findDevice(connectedDevices, *device) ) {
         return false;
     }
@@ -94,7 +94,7 @@ bool DBTAdapter::addConnectedDevice(const std::shared_ptr<DBTDevice> & device) n
 }
 
 bool DBTAdapter::removeConnectedDevice(const DBTDevice & device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_connectedDevices); // RAII-style acquire and relinquish via destructor
     for (auto it = connectedDevices.begin(); it != connectedDevices.end(); ) {
         if ( nullptr != *it && device == **it ) {
             it = connectedDevices.erase(it);
@@ -109,7 +109,7 @@ bool DBTAdapter::removeConnectedDevice(const DBTDevice & device) noexcept {
 int DBTAdapter::disconnectAllDevices(const HCIStatusCode reason) {
     std::vector<std::shared_ptr<DBTDevice>> devices;
     {
-        const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+        const std::lock_guard<std::recursive_mutex> lock(mtx_connectedDevices); // RAII-style acquire and relinquish via destructor
         devices = connectedDevices; // copy!
     }
     const int count = devices.size();
@@ -122,7 +122,7 @@ int DBTAdapter::disconnectAllDevices(const HCIStatusCode reason) {
 }
 
 std::shared_ptr<DBTDevice> DBTAdapter::findConnectedDevice (EUI48 const & mac, const BDAddressType macType) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_connectedDevices); // RAII-style acquire and relinquish via destructor
     return findDevice(connectedDevices, mac, macType);
 }
 
@@ -230,14 +230,14 @@ DBTAdapter::~DBTAdapter() {
     statusListenerList.clear();
 
     poweredOff();
-    sharedDevices.clear();
-
+    {
+        const std::lock_guard<std::recursive_mutex> lock(mtx_sharedDevices); // RAII-style acquire and relinquish via destructor
+        sharedDevices.clear();
+    }
     DBG_PRINT("DBTAdapter::dtor: XXX");
 }
 
 void DBTAdapter::poweredOff() {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_hci); // RAII-style acquire and relinquish via destructor
-
     DBG_PRINT("DBTAdapter::poweredOff: ... %p %s", this, toString(false).c_str());
     keepDiscoveringAlive = false;
 
@@ -254,11 +254,18 @@ void DBTAdapter::poweredOff() {
 }
 
 void DBTAdapter::printSharedPtrListOfDevices() noexcept {
-    const std::lock_guard<std::recursive_mutex> lock0(mtx_deviceReferences);
-
-    printSharedPtrList("SharedDevices", sharedDevices);
-    printSharedPtrList("DiscoveredDevices", discoveredDevices);
-    printSharedPtrList("ConnectedDevices", connectedDevices);
+    {
+        const std::lock_guard<std::recursive_mutex> lock0(mtx_sharedDevices);
+        printSharedPtrList("SharedDevices", sharedDevices);
+    }
+    {
+        const std::lock_guard<std::recursive_mutex> lock0(mtx_discoveredDevices);
+        printSharedPtrList("DiscoveredDevices", discoveredDevices);
+    }
+    {
+        const std::lock_guard<std::recursive_mutex> lock0(mtx_connectedDevices);
+        printSharedPtrList("ConnectedDevices", connectedDevices);
+    }
 }
 
 std::shared_ptr<NameAndShortName> DBTAdapter::setLocalName(const std::string &name, const std::string &short_name) noexcept {
@@ -548,12 +555,12 @@ exit:
 }
 
 std::shared_ptr<DBTDevice> DBTAdapter::findDiscoveredDevice (EUI48 const & mac, const BDAddressType macType) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(const_cast<DBTAdapter*>(this)->mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(const_cast<DBTAdapter*>(this)->mtx_discoveredDevices); // RAII-style acquire and relinquish via destructor
     return findDevice(discoveredDevices, mac, macType);
 }
 
 bool DBTAdapter::addDiscoveredDevice(std::shared_ptr<DBTDevice> const &device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_discoveredDevices); // RAII-style acquire and relinquish via destructor
     if( nullptr != findDevice(discoveredDevices, *device) ) {
         // already discovered
         return false;
@@ -563,7 +570,7 @@ bool DBTAdapter::addDiscoveredDevice(std::shared_ptr<DBTDevice> const &device) n
 }
 
 bool DBTAdapter::removeDiscoveredDevice(const DBTDevice & device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_discoveredDevices); // RAII-style acquire and relinquish via destructor
     for (auto it = discoveredDevices.begin(); it != discoveredDevices.end(); ) {
         if ( nullptr != *it && device == **it ) {
             it = discoveredDevices.erase(it);
@@ -577,20 +584,20 @@ bool DBTAdapter::removeDiscoveredDevice(const DBTDevice & device) noexcept {
 
 
 int DBTAdapter::removeDiscoveredDevices() noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_discoveredDevices); // RAII-style acquire and relinquish via destructor
     int res = discoveredDevices.size();
     discoveredDevices.clear();
     return res;
 }
 
 std::vector<std::shared_ptr<DBTDevice>> DBTAdapter::getDiscoveredDevices() const noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(const_cast<DBTAdapter*>(this)->mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(const_cast<DBTAdapter*>(this)->mtx_discoveredDevices); // RAII-style acquire and relinquish via destructor
     std::vector<std::shared_ptr<DBTDevice>> res = discoveredDevices;
     return res;
 }
 
 bool DBTAdapter::addSharedDevice(std::shared_ptr<DBTDevice> const &device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_sharedDevices); // RAII-style acquire and relinquish via destructor
     if( nullptr != findDevice(sharedDevices, *device) ) {
         // already shared
         return false;
@@ -600,12 +607,12 @@ bool DBTAdapter::addSharedDevice(std::shared_ptr<DBTDevice> const &device) noexc
 }
 
 std::shared_ptr<DBTDevice> DBTAdapter::getSharedDevice(const DBTDevice & device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_sharedDevices); // RAII-style acquire and relinquish via destructor
     return findDevice(sharedDevices, device);
 }
 
 void DBTAdapter::removeSharedDevice(const DBTDevice & device) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_sharedDevices); // RAII-style acquire and relinquish via destructor
     for (auto it = sharedDevices.begin(); it != sharedDevices.end(); ) {
         if ( nullptr != *it && device == **it ) {
             it = sharedDevices.erase(it);
@@ -617,13 +624,12 @@ void DBTAdapter::removeSharedDevice(const DBTDevice & device) noexcept {
 }
 
 std::shared_ptr<DBTDevice> DBTAdapter::findSharedDevice (EUI48 const & mac, const BDAddressType macType) noexcept {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
+    const std::lock_guard<std::recursive_mutex> lock(mtx_sharedDevices); // RAII-style acquire and relinquish via destructor
     return findDevice(sharedDevices, mac, macType);
 }
 
 void DBTAdapter::removeDevice(DBTDevice & device) {
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
-
+    const std::lock_guard<std::recursive_mutex> lock(mtx_sharedDevices); // RAII-style acquire and relinquish via destructor
     device.disconnect(false /* fromDisconnectCB */, false /* ioErrorCause */, HCIStatusCode::REMOTE_USER_TERMINATED_CONNECTION);
     removeConnectedDevice(device); // usually done in DBTAdapter::mgmtEvDeviceDisconnectedHCI
     removeDiscoveredDevice(device); // usually done in DBTAdapter::mgmtEvDeviceDisconnectedHCI
@@ -799,8 +805,6 @@ bool DBTAdapter::mgmtEvDeviceConnectedHCI(std::shared_ptr<MgmtEvent> e) {
         ad_report.setAddress( event.getAddress() );
         ad_report.read_data(event.getData(), event.getDataSize());
     }
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
-
     int new_connect = 0;
     std::shared_ptr<DBTDevice> device = findConnectedDevice(event.getAddress(), event.getAddressType());
     if( nullptr == device ) {
@@ -871,7 +875,6 @@ bool DBTAdapter::mgmtEvDeviceConnectedHCI(std::shared_ptr<MgmtEvent> e) {
 bool DBTAdapter::mgmtEvConnectFailedHCI(std::shared_ptr<MgmtEvent> e) {
     COND_PRINT(debug_event, "DBTAdapter::EventHCI:ConnectFailed: %s", e->toString().c_str());
     const MgmtEvtDeviceConnectFailed &event = *static_cast<const MgmtEvtDeviceConnectFailed *>(e.get());
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
 
     std::shared_ptr<DBTDevice> device = findConnectedDevice(event.getAddress(), event.getAddressType());
     if( nullptr != device ) {
@@ -906,7 +909,6 @@ bool DBTAdapter::mgmtEvConnectFailedHCI(std::shared_ptr<MgmtEvent> e) {
 
 bool DBTAdapter::mgmtEvDeviceDisconnectedHCI(std::shared_ptr<MgmtEvent> e) {
     const MgmtEvtDeviceDisconnected &event = *static_cast<const MgmtEvtDeviceDisconnected *>(e.get());
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
 
     std::shared_ptr<DBTDevice> device = findConnectedDevice(event.getAddress(), event.getAddressType());
     if( nullptr != device ) {
@@ -966,7 +968,6 @@ bool DBTAdapter::mgmtEvDeviceFoundHCI(std::shared_ptr<MgmtEvent> e) {
         eir->setRSSI( deviceFoundEvent.getRSSI() );
         eir->read_data(deviceFoundEvent.getData(), deviceFoundEvent.getDataSize());
     } // else: Sourced from HCIHandler via LE_ADVERTISING_REPORT (default!)
-    const std::lock_guard<std::recursive_mutex> lock(mtx_deviceReferences); // RAII-style acquire and relinquish via destructor
 
     std::shared_ptr<DBTDevice> dev = findDiscoveredDevice(eir->getAddress(), eir->getAddressType());
     if( nullptr != dev ) {