Skip to content

Commit

Permalink
[darwin] Race when BleConnection stop is called (#25522)
Browse files Browse the repository at this point in the history
  • Loading branch information
@vivien-apple @pull
vivien-apple authored and pull[bot] committed Feb 16, 2024
1 parent 307143b commit 3bbc8ae
Showing 1 changed file with 80 additions and 49 deletions.
129 changes: 80 additions & 49 deletions src/platform/Darwin/BleConnectionDelegateImpl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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;
Expand All @@ -58,7 +60,8 @@ @interface BleConnection : NSObject <CBCentralManagerDelegate, CBPeripheralDeleg
@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;
Expand All @@ -72,57 +75,80 @@ - (void)update;
namespace DeviceLayer {
namespace Internal {
BleConnection * ble;
dispatch_queue_t bleWorkQueue;

void BleConnectionDelegateImpl::NewConnection(
Ble::BleLayer * bleLayer, void * appState, const SetupDiscriminator & deviceDiscriminator)
{
assertChipStackLockedByCurrentThread();

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;
}

// 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 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
Expand All @@ -134,15 +160,14 @@ @interface BleConnection ()

@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];
Expand All @@ -159,9 +184,9 @@ - (id)init
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;
Expand Down Expand Up @@ -409,12 +434,28 @@ - (void)start
- (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
Expand Down Expand Up @@ -445,16 +486,6 @@ - (void)connect:(CBPeripheral *)peripheral
[_centralManager connectPeripheral:peripheral options:nil];
}

- (void)disconnect
{
if (!_centralManager || !_peripheral) {
return;
}

_mBleLayer->CloseAllBleConnections();
_peripheral = nil;
}

- (void)update
{
[_cachedPeripherals removeAllObjects];
Expand Down

0 comments on commit 3bbc8ae

Please sign in to comment.