[darwin] Race when BleConnection stop is called (#25522)
diff --git a/src/platform/Darwin/BleConnectionDelegateImpl.mm b/src/platform/Darwin/BleConnectionDelegateImpl.mm
index d696230..6997366 100644
--- a/src/platform/Darwin/BleConnectionDelegateImpl.mm
+++ b/src/platform/Darwin/BleConnectionDelegateImpl.mm
@@ -32,6 +32,7 @@
#include <lib/support/logging/CHIPLogging.h>
#include <platform/CHIPDeviceLayer.h>
#include <platform/Darwin/BleConnectionDelegate.h>
+#include <platform/LockTracker.h>
#include <setup_payload/SetupPayload.h>
#import "UUIDHelper.h"
@@ -40,11 +41,12 @@
constexpr uint64_t kScanningWithDiscriminatorTimeoutInSeconds = 60;
constexpr uint64_t kScanningWithoutDiscriminatorTimeoutInSeconds = 120;
+constexpr const char * kBleWorkQueueName = "org.csa-iot.matter.framework.ble.workqueue";
@interface BleConnection : NSObject <CBCentralManagerDelegate, CBPeripheralDelegate>
-@property (strong, nonatomic) dispatch_queue_t workQueue;
@property (strong, nonatomic) dispatch_queue_t chipWorkQueue;
+@property (strong, nonatomic) dispatch_queue_t workQueue;
@property (strong, nonatomic) CBCentralManager * centralManager;
@property (strong, nonatomic) CBPeripheral * peripheral;
@property (strong, nonatomic) CBUUID * shortServiceUUID;
@@ -58,7 +60,8 @@
@property (unsafe_unretained, nonatomic) BleConnectionDelegate::OnConnectionErrorFunct onConnectionError;
@property (unsafe_unretained, nonatomic) chip::Ble::BleLayer * mBleLayer;
-- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator;
+- (id)initWithQueue:(dispatch_queue_t)queue;
+- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator queue:(dispatch_queue_t)queue;
- (void)setBleLayer:(chip::Ble::BleLayer *)bleLayer;
- (void)start;
- (void)stop;
@@ -72,57 +75,80 @@
namespace DeviceLayer {
namespace Internal {
BleConnection * ble;
+ dispatch_queue_t bleWorkQueue;
void BleConnectionDelegateImpl::NewConnection(
Ble::BleLayer * bleLayer, void * appState, const SetupDiscriminator & deviceDiscriminator)
{
- ChipLogProgress(Ble, "%s", __FUNCTION__);
+ assertChipStackLockedByCurrentThread();
- // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
- // creating a brand new connection but update the discriminator and the ble layer members.
- if (ble and ![ble hasDiscriminator]) {
+ ChipLogProgress(Ble, "%s", __FUNCTION__);
+ if (!bleWorkQueue) {
+ bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
+ }
+
+ dispatch_async(bleWorkQueue, ^{
+ // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
+ // creating a brand new connection but update the discriminator and the ble layer members.
+ if (ble and ![ble hasDiscriminator]) {
+ [ble setBleLayer:bleLayer];
+ ble.appState = appState;
+ ble.onConnectionComplete = OnConnectionComplete;
+ ble.onConnectionError = OnConnectionError;
+ [ble updateWithDiscriminator:deviceDiscriminator];
+ return;
+ }
+
+ [ble stop];
+ ble = [[BleConnection alloc] initWithDiscriminator:deviceDiscriminator queue:bleWorkQueue];
[ble setBleLayer:bleLayer];
ble.appState = appState;
ble.onConnectionComplete = OnConnectionComplete;
ble.onConnectionError = OnConnectionError;
- [ble updateWithDiscriminator:deviceDiscriminator];
- return;
- }
-
- CancelConnection();
- ble = [[BleConnection alloc] initWithDiscriminator:deviceDiscriminator];
- [ble setBleLayer:bleLayer];
- ble.appState = appState;
- ble.onConnectionComplete = OnConnectionComplete;
- ble.onConnectionError = OnConnectionError;
- ble.centralManager = [ble.centralManager initWithDelegate:ble queue:ble.workQueue];
+ ble.centralManager = [ble.centralManager initWithDelegate:ble queue:bleWorkQueue];
+ });
}
void BleConnectionDelegateImpl::PrepareConnection()
{
- ChipLogProgress(Ble, "%s", __FUNCTION__);
+ assertChipStackLockedByCurrentThread();
- // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
- // creating a brand new connection but clear the cache and reset the timer.
- if (ble and ![ble hasDiscriminator]) {
- [ble update];
- return;
+ ChipLogProgress(Ble, "%s", __FUNCTION__);
+ if (!bleWorkQueue) {
+ bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
}
- CancelConnection();
- ble = [[BleConnection alloc] init];
- ble.onConnectionComplete = OnConnectionComplete;
- ble.onConnectionError = OnConnectionError;
- ble.centralManager = [ble.centralManager initWithDelegate:ble queue:ble.workQueue];
+ dispatch_async(bleWorkQueue, ^{
+ // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
+ // creating a brand new connection but clear the cache and reset the timer.
+ if (ble and ![ble hasDiscriminator]) {
+ [ble update];
+ return;
+ }
+
+ [ble stop];
+ ble = [[BleConnection alloc] initWithQueue:bleWorkQueue];
+ ble.onConnectionComplete = OnConnectionComplete;
+ ble.onConnectionError = OnConnectionError;
+ ble.centralManager = [ble.centralManager initWithDelegate:ble queue:bleWorkQueue];
+ });
}
CHIP_ERROR BleConnectionDelegateImpl::CancelConnection()
{
+ assertChipStackLockedByCurrentThread();
+
ChipLogProgress(Ble, "%s", __FUNCTION__);
- if (ble) {
+ if (bleWorkQueue == nil) {
+ return CHIP_NO_ERROR;
+ }
+
+ dispatch_async(bleWorkQueue, ^{
[ble stop];
ble = nil;
- }
+ });
+
+ bleWorkQueue = nil;
return CHIP_NO_ERROR;
}
} // namespace Internal
@@ -134,15 +160,14 @@
@implementation BleConnection
-- (id)init
+- (id)initWithQueue:(dispatch_queue_t)queue
{
self = [super init];
if (self) {
self.shortServiceUUID = [UUIDHelper GetShortestServiceUUID:&chip::Ble::CHIP_BLE_SVC_ID];
- _workQueue
- = dispatch_queue_create("org.csa-iot.matter.framework.ble.workqueue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
_chipWorkQueue = chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue();
- _timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, _workQueue);
+ _workQueue = queue;
+ _timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, queue);
_centralManager = [CBCentralManager alloc];
_found = false;
_cachedPeripherals = [[NSMutableDictionary alloc] init];
@@ -159,9 +184,9 @@
return self;
}
-- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator
+- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator queue:(dispatch_queue_t)queue
{
- self = [self init];
+ self = [self initWithQueue:queue];
if (self) {
_deviceDiscriminator = deviceDiscriminator;
_hasDeviceDiscriminator = true;
@@ -409,12 +434,28 @@
- (void)stop
{
[self stopScanning];
- [self disconnect];
[_cachedPeripherals removeAllObjects];
_cachedPeripherals = nil;
- _centralManager.delegate = nil;
- _centralManager = nil;
- _peripheral = nil;
+
+ if (!_centralManager || !_peripheral) {
+ return;
+ }
+
+ // Properly closing the underlying ble connections needs to happens
+ // on the chip work queue. At the same time the SDK is trying to
+ // properly unsubscribe and shutdown the connection, so if we nullify
+ // the centralManager and the peripheral members too early it won't be
+ // able to reach those.
+ // This is why closing connections happens as 2 async steps.
+ dispatch_async(_chipWorkQueue, ^{
+ _mBleLayer->CloseAllBleConnections();
+
+ dispatch_async(_workQueue, ^{
+ _centralManager.delegate = nil;
+ _centralManager = nil;
+ _peripheral = nil;
+ });
+ });
}
- (void)startScanning
@@ -445,16 +486,6 @@
[_centralManager connectPeripheral:peripheral options:nil];
}
-- (void)disconnect
-{
- if (!_centralManager || !_peripheral) {
- return;
- }
-
- _mBleLayer->CloseAllBleConnections();
- _peripheral = nil;
-}
-
- (void)update
{
[_cachedPeripherals removeAllObjects];
