Skip to content

Commit

Permalink
Make some more safety improvements to dispatch to the Matter queue. (#…
Browse files Browse the repository at this point in the history 
…24084)

The idea is to minimize the number of places that have to have
isRunning checks, and direct access to the Matter queue, by having
APIs that do those for you.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Oct 18, 2023
1 parent d151b0d commit aa918a4
Show file tree
Hide file tree
Showing 8 changed files with 183 additions and 185 deletions.
6 changes: 3 additions & 3 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ static void PurgeReadClientContainers(

// Destroy read clients in the work queue
[controller
asyncDispatchToMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
asyncDispatchToMatterQueue:^() {
for (MTRReadClientContainer * container in listToDelete) {
if (container.readClientPtr) {
Platform::Delete(container.readClientPtr);
Expand Down Expand Up @@ -193,7 +193,7 @@ static void CauseReadClientFailure(
[readClientContainersLock unlock];

[controller
asyncDispatchToMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
asyncDispatchToMatterQueue:^() {
for (MTRReadClientContainer * container in listToFail) {
// Send auto resubscribe request again by read clients, which must fail.
chip::app::ReadPrepareParams readParams;
Expand Down Expand Up @@ -1373,7 +1373,7 @@ - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
}

[self.deviceController
asyncDispatchToMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
asyncGetCommissionerOnMatterQueue:^(Controller::DeviceCommissioner * commissioner) {
auto resultCallback = ^(CHIP_ERROR status, const SetupPayload & payload) {
if (status != CHIP_NO_ERROR) {
dispatch_async(queue, ^{
Expand Down
31 changes: 11 additions & 20 deletions src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
LogRequestStart();

[device.deviceController
asyncDispatchToMatterQueue:^(chip::Controller::DeviceCommissioner *) {
asyncDispatchToMatterQueue:^() {
CHIP_ERROR err = action(mSuccess, mFailure);
if (err != CHIP_NO_ERROR) {
NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
Expand All @@ -159,31 +159,22 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
{
LogRequestStart();

BOOL ok = [device.deviceController
getSessionForCommissioneeDevice:device.nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
MaybeDoAction(exchangeManager, session, error);
}];

if (ok == NO) {
OnFailureFn(this, CHIP_ERROR_INCORRECT_STATE);
}
[device.deviceController getSessionForCommissioneeDevice:device.nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
MaybeDoAction(exchangeManager, session, error);
}];
}

void ActionWithNodeID(chip::NodeId nodeID, MTRDeviceController * controller)
{
LogRequestStart();

BOOL ok = [controller getSessionForNode:nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
MaybeDoAction(exchangeManager, session, error);
}];

if (ok == NO) {
OnFailureFn(this, CHIP_ERROR_INCORRECT_STATE);
}
[controller getSessionForNode:nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
MaybeDoAction(exchangeManager, session, error);
}];
}

void LogRequestStart()
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/MTRClusterStateCacheContainer.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
}

[self.baseDevice.deviceController
asyncDispatchToMatterQueue:^(Controller::DeviceCommissioner *) {
asyncDispatchToMatterQueue:^() {
if (endpointID == nil && clusterID == nil) {
MTR_LOG_ERROR(
"Error: currently read from attribute cache does not support wildcards for both endpoint and cluster");
Expand Down
158 changes: 74 additions & 84 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dis
if ([self checkForInitError:(_operationalCredentialsDelegate != nullptr) logMsg:kErrorOperationalCredentialsInit]) {
return nil;
}
_operationalCredentialsDelegate->setChipWorkQueue(_chipWorkQueue);
}
return self;
}
Expand Down Expand Up @@ -544,13 +543,11 @@ - (void)removeDevice:(MTRDevice *)device

- (void)setDeviceControllerDelegate:(id<MTRDeviceControllerDelegate>)delegate queue:(dispatch_queue_t)queue
{
VerifyOrReturn([self checkIsRunning]);

dispatch_async(_chipWorkQueue, ^{
VerifyOrReturn([self checkIsRunning]);

self->_deviceControllerDelegateBridge->setDelegate(self, delegate, queue);
});
[self
asyncDispatchToMatterQueue:^() {
self->_deviceControllerDelegateBridge->setDelegate(self, delegate, queue);
}
errorHandler:nil];
}

- (BOOL)setOperationalCertificateIssuer:(nullable id<MTROperationalCertificateIssuer>)operationalCertificateIssuer
Expand Down Expand Up @@ -692,83 +689,79 @@ - (BOOL)_deviceBeingCommissionedOverBLE:(uint64_t)deviceID
return deviceProxy->GetDeviceTransportType() == chip::Transport::Type::kBle;
}

- (BOOL)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion
- (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion
{
if (![self checkIsRunning]) {
return NO;
}
[self
asyncGetCommissionerOnMatterQueue:^(chip::Controller::DeviceCommissioner * commissioner) {
auto connectionBridge = new MTRDeviceConnectionBridge(completion);

dispatch_async(_chipWorkQueue, ^{
NSError * error;
if (![self checkIsRunning:&error]) {
completion(nullptr, chip::NullOptional, error);
return;
// MTRDeviceConnectionBridge always delivers errors async via
// completion.
connectionBridge->connect(commissioner, nodeID);
}

auto connectionBridge = new MTRDeviceConnectionBridge(completion);

// MTRDeviceConnectionBridge always delivers errors async via
// completion.
connectionBridge->connect(self->_cppCommissioner, nodeID);
});

return YES;
errorHandler:^(NSError * error) {
completion(nullptr, chip::NullOptional, error);
}];
}

- (BOOL)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion
- (void)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion
{
if (![self checkIsRunning]) {
return NO;
}

dispatch_async(_chipWorkQueue, ^{
NSError * error;
if (![self checkIsRunning:&error]) {
completion(nullptr, chip::NullOptional, error);
return;
}
[self
asyncGetCommissionerOnMatterQueue:^(chip::Controller::DeviceCommissioner * commissioner) {
chip::CommissioneeDeviceProxy * deviceProxy;
CHIP_ERROR err = commissioner->GetDeviceBeingCommissioned(deviceID, &deviceProxy);
if (err != CHIP_NO_ERROR) {
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err]);
return;
}

chip::CommissioneeDeviceProxy * deviceProxy;
CHIP_ERROR err = self->_cppCommissioner->GetDeviceBeingCommissioned(deviceID, &deviceProxy);
if (err != CHIP_NO_ERROR) {
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err]);
return;
}
chip::Optional<chip::SessionHandle> session = deviceProxy->GetSecureSession();
if (!session.HasValue() || !session.Value()->AsSecureSession()->IsPASESession()) {
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
return;
}

chip::Optional<chip::SessionHandle> session = deviceProxy->GetSecureSession();
if (!session.HasValue() || !session.Value()->AsSecureSession()->IsPASESession()) {
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
return;
completion(deviceProxy->GetExchangeManager(), session, nil);
}

completion(deviceProxy->GetExchangeManager(), session, nil);
});

return YES;
errorHandler:^(NSError * error) {
completion(nullptr, chip::NullOptional, error);
}];
}

- (void)asyncDispatchToMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
errorHandler:(void (^)(NSError *))errorHandler
- (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
errorHandler:(nullable MTRDeviceErrorHandler)errorHandler
{
{
NSError * error;
if (![self checkIsRunning:&error]) {
errorHandler(error);
if (errorHandler != nil) {
errorHandler(error);
}
return;
}
}

dispatch_async(_chipWorkQueue, ^{
NSError * error;
if (![self checkIsRunning:&error]) {
errorHandler(error);
if (errorHandler != nil) {
errorHandler(error);
}
return;
}

block(self.cppCommissioner);
});
}

- (void)asyncDispatchToMatterQueue:(dispatch_block_t)block errorHandler:(nullable MTRDeviceErrorHandler)errorHandler
{
auto adapter = ^(chip::Controller::DeviceCommissioner *) {
block();
};
[self asyncGetCommissionerOnMatterQueue:adapter errorHandler:errorHandler];
}

- (void)syncRunOnWorkQueue:(SyncWorkQueueBlock)block error:(NSError * __autoreleasing *)error
{
VerifyOrReturn([self checkIsRunning:error]);
Expand All @@ -782,27 +775,22 @@ - (void)syncRunOnWorkQueue:(SyncWorkQueueBlock)block error:(NSError * __autorele
- (id)syncRunOnWorkQueueWithReturnValue:(SyncWorkQueueBlockWithReturnValue)block error:(NSError * __autoreleasing *)error
{
__block id rv = nil;

VerifyOrReturnValue([self checkIsRunning:error], rv);

dispatch_sync(_chipWorkQueue, ^{
VerifyOrReturn([self checkIsRunning:error]);
auto adapter = ^{
rv = block();
});
};

[self syncRunOnWorkQueue:adapter error:error];

return rv;
}

- (BOOL)syncRunOnWorkQueueWithBoolReturnValue:(SyncWorkQueueBlockWithBoolReturnValue)block error:(NSError * __autoreleasing *)error
{
__block BOOL success = NO;

VerifyOrReturnValue([self checkIsRunning:error], success);

dispatch_sync(_chipWorkQueue, ^{
VerifyOrReturn([self checkIsRunning:error]);
auto adapter = ^{
success = block();
});
};
[self syncRunOnWorkQueue:adapter error:error];

return success;
}
Expand Down Expand Up @@ -1001,22 +989,24 @@ - (BOOL)getBaseDevice:(uint64_t)deviceID queue:(dispatch_queue_t)queue completio

// We know getSessionForNode will return YES here, since we already checked
// that we are running.
return [self getSessionForNode:deviceID
completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) {
// Create an MTRBaseDevice for the node id involved, now that our
// CASE session is primed. We don't actually care about the session
// information here.
dispatch_async(queue, ^{
MTRBaseDevice * device;
if (error == nil) {
device = [[MTRBaseDevice alloc] initWithNodeID:@(deviceID) controller:self];
} else {
device = nil;
}
completion(device, error);
});
}];
[self getSessionForNode:deviceID
completion:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) {
// Create an MTRBaseDevice for the node id involved, now that our
// CASE session is primed. We don't actually care about the session
// information here.
dispatch_async(queue, ^{
MTRBaseDevice * device;
if (error == nil) {
device = [[MTRBaseDevice alloc] initWithNodeID:@(deviceID) controller:self];
} else {
device = nil;
}
completion(device, error);
});
}];

return YES;
}

- (BOOL)pairDevice:(uint64_t)deviceID
Expand Down
62 changes: 40 additions & 22 deletions src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,31 +108,36 @@ NS_ASSUME_NONNULL_BEGIN
/**
* Ensure we have a CASE session to the given node ID and then call the provided
* connection callback. This may be called on any queue (including the Matter
* event queue) and will always call the provided connection callback on the
* Matter queue, asynchronously. Consumers must be prepared to run on the
* Matter queue (an in particular must not use any APIs that will try to do sync
* dispatch to the Matter queue).
*
* If the controller is not running when this function is called, will return NO
* and never invoke the completion. If the controller is not running when the
* async dispatch on the Matter queue would happen, an error will be dispatched
* to the completion handler.
* event queue) and on success will always call the provided connection callback
* on the Matter queue, asynchronously. Consumers must be prepared to run on
* the Matter queue (an in particular must not use any APIs that will try to do
* sync dispatch to the Matter queue).
*
* If the controller is not running when this function is called, it will
* synchronously invoke the completion with an error, on whatever queue
* getSessionForNode was called on.
*
* If the controller is not running when the async dispatch on the Matter queue
* happens, the completion will be invoked with an error on the Matter queue.
*/
- (BOOL)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion;
- (void)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion;

/**
* Get a session for the commissionee device with the given device id. This may
* be called on any queue (including the Matter event queue) and will always
* call the provided connection callback on the Matter queue, asynchronously.
* Consumers must be prepared to run on the Matter queue (an in particular must
* not use any APIs that will try to do sync dispatch to the Matter queue).
*
* If the controller is not running when this function is called, will return NO
* and never invoke the completion. If the controller is not running when the
* async dispatch on the Matter queue would happen, an error will be dispatched
* to the completion handler.
* be called on any queue (including the Matter event queue) and on success will
* always call the provided connection callback on the Matter queue,
* asynchronously. Consumers must be prepared to run on the Matter queue (an in
* particular must not use any APIs that will try to do sync dispatch to the
* Matter queue).
*
* If the controller is not running when this function is called, it will
* synchronously invoke the completion with an error, on whatever queue
* getSessionForCommissioneeDevice was called on.
*
* If the controller is not running when the async dispatch on the Matter queue
* happens, the completion will be invoked with an error on the Matter queue.
*/
- (BOOL)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion;
- (void)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion;

/**
* Invalidate the CASE session for the given node ID. This is a temporary thing
Expand All @@ -150,9 +155,22 @@ NS_ASSUME_NONNULL_BEGIN
*
* The DeviceCommissioner pointer passed to the callback should only be used
* synchronously during the callback invocation.
*
* If the error handler is nil, failure to run the block will be silent.
*/
- (void)asyncGetCommissionerOnMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
errorHandler:(nullable MTRDeviceErrorHandler)errorHandler;

/**
* Try to asynchronously dispatch the given block on the Matter queue. If the
* controller is not running either at call time or when the block would be
* about to run, the provided error handler will be called with an error. Note
* that this means the error handler might be called on an arbitrary queue, and
* might be called before this function returns or after it returns.
*
* If the error handler is nil, failure to run the block will be silent.
*/
- (void)asyncDispatchToMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
errorHandler:(void (^)(NSError *))errorHandler;
- (void)asyncDispatchToMatterQueue:(dispatch_block_t)block errorHandler:(nullable MTRDeviceErrorHandler)errorHandler;

/**
* Get an MTRBaseDevice for the given node id. This exists to allow subclasses
Expand Down
Loading

0 comments on commit aa918a4

Please sign in to comment.