From 34574614f32ee56f79cb47ddeae7c51c471c3bc6 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 30 Nov 2022 22:00:25 -0500 Subject: [PATCH] Make sure to not dispatch to shut-down queues in MTROTAProviderDelegateBridge. (#23841) * Make sure we call our delegate callbacks on a queue different from the Matter work queue. * Assert that we are on the Matter work queue in all the places where we should be. * Make sure to dispatch to the Matter work queue via the controller we were dealing with, so dispatch does not happen if that controller has shut down. * Reset OTA transfers when the controller the transfer is associated with shuts down. * Ensure that async callbacks for a stale transfer don't affect a current transfer. Fixes https://github.com/project-chip/connectedhomeip/issues/22541 --- .../CHIP/MTRDeviceControllerFactory.h | 3 + .../CHIP/MTRDeviceControllerFactory.mm | 19 +- .../CHIP/MTROTAProviderDelegateBridge.h | 9 +- .../CHIP/MTROTAProviderDelegateBridge.mm | 331 ++++++++++++------ 4 files changed, 239 insertions(+), 123 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h index 97e7ca28749050..2169aede2861a9 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h @@ -44,6 +44,9 @@ MTR_NEWLY_AVAILABLE /* * OTA Provider delegate to be called when an OTA Requestor is requesting a software update. * Defaults to nil. + * + * Calls to this delegate can happen on an arbitrary thread, but will not happen + * concurrently. */ @property (nonatomic, strong, nullable) id otaProviderDelegate; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 81f53c4f45d74c..f089a000fdd41c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -674,8 +674,11 @@ - (MTRDeviceController * _Nullable)maybeInitializeOTAProvider:(MTRDeviceControll VerifyOrReturnValue(_otaProviderDelegateBridge != nil, controller); VerifyOrReturnValue([_controllers count] == 1, controller); - auto systemState = _controllerFactory->GetSystemState(); - CHIP_ERROR err = _otaProviderDelegateBridge->Init(systemState->SystemLayer(), systemState->ExchangeMgr()); + __block CHIP_ERROR err; + dispatch_sync(_chipWorkQueue, ^{ + auto systemState = _controllerFactory->GetSystemState(); + err = _otaProviderDelegateBridge->Init(systemState->SystemLayer(), systemState->ExchangeMgr()); + }); if (CHIP_NO_ERROR != err) { MTR_LOG_ERROR("Failed to init provider delegate bridge: %" CHIP_ERROR_FORMAT, err.Format()); [controller shutdown]; @@ -711,19 +714,23 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller [_controllers removeObject:controller]; if ([_controllers count] == 0) { - if (_otaProviderDelegateBridge) { - _otaProviderDelegateBridge->Shutdown(); - } - // That was our last controller. Stop the event loop before it // shuts down, because shutdown of the last controller will tear // down most of the world. DeviceLayer::PlatformMgrImpl().StopEventLoopTask(); + if (_otaProviderDelegateBridge) { + _otaProviderDelegateBridge->Shutdown(); + } + [controller shutDownCppController]; } else { // Do the controller shutdown on the Matter work queue. dispatch_sync(_chipWorkQueue, ^{ + if (_otaProviderDelegateBridge) { + _otaProviderDelegateBridge->ControllerShuttingDown(controller); + } + [controller shutDownCppController]; }); } diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h index 3c69407046e6fa..a4b69f231dacd7 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.h @@ -28,8 +28,15 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele ~MTROTAProviderDelegateBridge(); CHIP_ERROR Init(chip::System::Layer * systemLayer, chip::Messaging::ExchangeManager * exchangeManager); + + // Shutdown must be called after the event loop has been stopped, since it + // touches Matter objects. void Shutdown(); + // ControllerShuttingDown must be called on the Matter work queue, since it + // touches Matter objects. + void ControllerShuttingDown(MTRDeviceController * controller); + void HandleQueryImage( chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, const chip::app::Clusters::OtaSoftwareUpdateProvider::Commands::QueryImage::DecodableType & commandData) override; @@ -60,7 +67,7 @@ class MTROTAProviderDelegateBridge : public chip::app::Clusters::OTAProviderDele MTROTASoftwareUpdateProviderClusterNotifyUpdateAppliedParams * commandParams); _Nullable id mDelegate; - dispatch_queue_t mWorkQueue; + dispatch_queue_t mDelegateNotificationQueue; }; NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm index 5de6041ca67790..7d5e8c165669bd 100644 --- a/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm +++ b/src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm @@ -17,16 +17,19 @@ #import "MTROTAProviderDelegateBridge.h" #import "MTRDeviceControllerFactory_Internal.h" +#import "MTRDeviceController_Internal.h" #import "NSDataSpanConversion.h" #import "NSStringSpanConversion.h" #include +#include #include #include #include #include #include +#include #include #include @@ -49,6 +52,8 @@ CHIP_ERROR PrepareForTransfer(FabricIndex fabricIndex, NodeId nodeId) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -61,6 +66,8 @@ CHIP_ERROR PrepareForTransfer(FabricIndex fabricIndex, NodeId nodeId) CHIP_ERROR Init(System::Layer * systemLayer, Messaging::ExchangeManager * exchangeMgr) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mSystemLayer == nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mExchangeMgr == nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(systemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -70,13 +77,14 @@ CHIP_ERROR Init(System::Layer * systemLayer, Messaging::ExchangeManager * exchan mSystemLayer = systemLayer; mExchangeMgr = exchangeMgr; - mWorkQueue = DeviceLayer::PlatformMgrImpl().GetWorkQueue(); return CHIP_NO_ERROR; } CHIP_ERROR Shutdown() { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mExchangeMgr != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -84,25 +92,39 @@ CHIP_ERROR Shutdown() mExchangeMgr = nullptr; mSystemLayer = nullptr; - mWorkQueue = nil; + mDelegateNotificationQueue = nil; ResetState(); return CHIP_NO_ERROR; } - void SetDelegate(id delegate) + void ControllerShuttingDown(MTRDeviceController * controller) + { + assertChipStackLockedByCurrentThread(); + + if (mInitialized && mFabricIndex.Value() == controller.fabricIndex) { + ResetState(); + } + } + + void SetDelegate(id delegate, dispatch_queue_t delegateNotificationQueue) { if (delegate) { mDelegate = delegate; + mDelegateNotificationQueue = delegateNotificationQueue; } else { ResetState(); + mDelegate = nil; + mDelegateNotificationQueue = nil; } } private: CHIP_ERROR OnMessageToSend(TransferSession::OutputEvent & event) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDelegate != nil, CHIP_ERROR_INCORRECT_STATE); @@ -120,6 +142,8 @@ CHIP_ERROR OnMessageToSend(TransferSession::OutputEvent & event) CHIP_ERROR OnTransferSessionBegin(TransferSession::OutputEvent & event) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mFabricIndex.HasValue(), CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mNodeId.HasValue(), CHIP_ERROR_INCORRECT_STATE); uint16_t fdl = 0; @@ -128,33 +152,48 @@ CHIP_ERROR OnTransferSessionBegin(TransferSession::OutputEvent & event) auto fileDesignator = [[NSString alloc] initWithBytes:fd length:fdl encoding:NSUTF8StringEncoding]; auto offset = @(mTransfer.GetStartOffset()); + + auto * controller = [[MTRDeviceControllerFactory sharedInstance] runningControllerForFabricIndex:mFabricIndex.Value()]; + VerifyOrReturnError(controller != nil, CHIP_ERROR_INCORRECT_STATE); + + auto transferGeneration = mTransferGeneration; + auto completionHandler = ^(NSError * _Nullable error) { - dispatch_async(mWorkQueue, ^{ - if (error != nil) { - LogErrorOnFailure([MTRError errorToCHIPErrorCode:error]); - LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnknown)); - return; + [controller + asyncDispatchToMatterQueue:^(chip::Controller::DeviceCommissioner *) { + assertChipStackLockedByCurrentThread(); + + if (!mInitialized || mTransferGeneration != transferGeneration) { + // Callback for a stale transfer. + return; + } + + if (error != nil) { + LogErrorOnFailure([MTRError errorToCHIPErrorCode:error]); + LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnknown)); + return; + } + + // bdx::TransferSession will automatically reject a transfer if there are no + // common supported control modes. It will also default to the smaller + // block size. + TransferSession::TransferAcceptData acceptData; + acceptData.ControlMode = bdx::TransferControlFlags::kReceiverDrive; + acceptData.MaxBlockSize = mTransfer.GetTransferBlockSize(); + acceptData.StartOffset = mTransfer.GetStartOffset(); + acceptData.Length = mTransfer.GetTransferLength(); + + LogErrorOnFailure(mTransfer.AcceptTransfer(acceptData)); } - - // bdx::TransferSession will automatically reject a transfer if there are no - // common supported control modes. It will also default to the smaller - // block size. - TransferSession::TransferAcceptData acceptData; - acceptData.ControlMode = bdx::TransferControlFlags::kReceiverDrive; - acceptData.MaxBlockSize = mTransfer.GetTransferBlockSize(); - acceptData.StartOffset = mTransfer.GetStartOffset(); - acceptData.Length = mTransfer.GetTransferLength(); - - LogErrorOnFailure(mTransfer.AcceptTransfer(acceptData)); - }); + errorHandler:^(NSError *) { + // Not much we can do here + }]; }; - auto * controller = [[MTRDeviceControllerFactory sharedInstance] runningControllerForFabricIndex:mFabricIndex.Value()]; - VerifyOrReturnError(controller != nil, CHIP_ERROR_INCORRECT_STATE); auto nodeId = @(mNodeId.Value()); auto strongDelegate = mDelegate; - dispatch_async(mWorkQueue, ^{ + dispatch_async(mDelegateNotificationQueue, ^{ if ([strongDelegate respondsToSelector:@selector (handleBDXTransferSessionBeginForNodeID:controller:fileDesignator:offset:completion:)]) { [strongDelegate handleBDXTransferSessionBeginForNodeID:nodeId @@ -176,6 +215,8 @@ CHIP_ERROR OnTransferSessionBegin(TransferSession::OutputEvent & event) CHIP_ERROR OnTransferSessionEnd(TransferSession::OutputEvent & event) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mFabricIndex.HasValue(), CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mNodeId.HasValue(), CHIP_ERROR_INCORRECT_STATE); @@ -191,7 +232,7 @@ CHIP_ERROR OnTransferSessionEnd(TransferSession::OutputEvent & event) auto nodeId = @(mNodeId.Value()); auto strongDelegate = mDelegate; - dispatch_async(mWorkQueue, ^{ + dispatch_async(mDelegateNotificationQueue, ^{ [strongDelegate handleBDXTransferSessionEndForNodeID:nodeId controller:controller error:[MTRError errorForCHIPErrorCode:error]]; @@ -203,6 +244,8 @@ CHIP_ERROR OnTransferSessionEnd(TransferSession::OutputEvent & event) CHIP_ERROR OnBlockQuery(TransferSession::OutputEvent & event) { + assertChipStackLockedByCurrentThread(); + VerifyOrReturnError(mFabricIndex.HasValue(), CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mNodeId.HasValue(), CHIP_ERROR_INCORRECT_STATE); @@ -214,34 +257,48 @@ CHIP_ERROR OnBlockQuery(TransferSession::OutputEvent & event) bytesToSkip = @(event.bytesToSkip.BytesToSkip); } - auto completionHandler = ^(NSData * _Nullable data, BOOL isEOF) { - dispatch_async(mWorkQueue, ^{ - if (data == nil) { - LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnknown)); - return; - } + auto * controller = [[MTRDeviceControllerFactory sharedInstance] runningControllerForFabricIndex:mFabricIndex.Value()]; + VerifyOrReturnError(controller != nil, CHIP_ERROR_INCORRECT_STATE); - TransferSession::BlockData blockData; - blockData.Data = static_cast([data bytes]); - blockData.Length = static_cast([data length]); - blockData.IsEof = isEOF; + auto transferGeneration = mTransferGeneration; - CHIP_ERROR err = mTransfer.PrepareBlock(blockData); - if (CHIP_NO_ERROR != err) { - LogErrorOnFailure(err); - LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnknown)); + auto completionHandler = ^(NSData * _Nullable data, BOOL isEOF) { + [controller + asyncDispatchToMatterQueue:^(chip::Controller::DeviceCommissioner *) { + assertChipStackLockedByCurrentThread(); + + if (!mInitialized || mTransferGeneration != transferGeneration) { + // Callback for a stale transfer. + return; + } + + if (data == nil) { + LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnknown)); + return; + } + + TransferSession::BlockData blockData; + blockData.Data = static_cast([data bytes]); + blockData.Length = static_cast([data length]); + blockData.IsEof = isEOF; + + CHIP_ERROR err = mTransfer.PrepareBlock(blockData); + if (CHIP_NO_ERROR != err) { + LogErrorOnFailure(err); + LogErrorOnFailure(mTransfer.AbortTransfer(bdx::StatusCode::kUnknown)); + } } - }); + errorHandler:^(NSError *) { + // Not much we can do here + }]; }; // TODO Handle MaxLength - auto * controller = [[MTRDeviceControllerFactory sharedInstance] runningControllerForFabricIndex:mFabricIndex.Value()]; - VerifyOrReturnError(controller != nil, CHIP_ERROR_INCORRECT_STATE); auto nodeId = @(mNodeId.Value()); auto strongDelegate = mDelegate; - dispatch_async(mWorkQueue, ^{ + dispatch_async(mDelegateNotificationQueue, ^{ if ([strongDelegate respondsToSelector:@selector (handleBDXQueryForNodeID:controller:blockSize:blockIndex:bytesToSkip:completion:)]) { [strongDelegate handleBDXQueryForNodeID:nodeId @@ -303,6 +360,8 @@ void HandleTransferSessionOutput(TransferSession::OutputEvent & event) override CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId) { + assertChipStackLockedByCurrentThread(); + if (mInitialized) { // Prevent a new node connection since another is active. VerifyOrReturnError(mFabricIndex.Value() == fabricIndex && mNodeId.Value() == nodeId, CHIP_ERROR_BUSY); @@ -321,11 +380,14 @@ CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId) void ResetState() { + assertChipStackLockedByCurrentThread(); + if (!mInitialized) { return; } Responder::ResetTransfer(); + ++mTransferGeneration; mFabricIndex.ClearValue(); mNodeId.ClearValue(); @@ -341,8 +403,14 @@ void ResetState() Optional mFabricIndex; Optional mNodeId; id mDelegate = nil; - dispatch_queue_t mWorkQueue = nil; + dispatch_queue_t mDelegateNotificationQueue = nil; Messaging::ExchangeManager * mExchangeMgr = nullptr; + + // Since we are a singleton, we get reused across transfers, but also have + // async calls that can happen. The transfer generation keeps track of + // which transfer we are currently doing, so we can ignore async calls + // attached to no-longer-running transfers. + uint64_t mTransferGeneration = 0; }; BdxOTASender gOtaSender; @@ -351,15 +419,15 @@ void ResetState() MTROTAProviderDelegateBridge::MTROTAProviderDelegateBridge(id delegate) : mDelegate(delegate) - , mWorkQueue(DeviceLayer::PlatformMgrImpl().GetWorkQueue()) + , mDelegateNotificationQueue(dispatch_queue_create("com.csa.matter.framework.otaprovider.workqueue", DISPATCH_QUEUE_SERIAL)) { - gOtaSender.SetDelegate(delegate); + gOtaSender.SetDelegate(delegate, mDelegateNotificationQueue); Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, this); } MTROTAProviderDelegateBridge::~MTROTAProviderDelegateBridge() { - gOtaSender.SetDelegate(nil); + gOtaSender.SetDelegate(nil, nil); Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, nullptr); } @@ -370,6 +438,11 @@ void ResetState() void MTROTAProviderDelegateBridge::Shutdown() { gOtaSender.Shutdown(); } +void MTROTAProviderDelegateBridge::ControllerShuttingDown(MTRDeviceController * controller) +{ + gOtaSender.ControllerShuttingDown(controller); +} + namespace { // Return false if we could not get peer node info (a running controller for // the fabric and a node id). In that case we will have already added an @@ -456,6 +529,8 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath void MTROTAProviderDelegateBridge::HandleQueryImage( CommandHandler * commandObj, const ConcreteCommandPath & commandPath, const Commands::QueryImage::DecodableType & commandData) { + assertChipStackLockedByCurrentThread(); + NodeId nodeId; MTRDeviceController * controller; if (!GetPeerNodeInfo(commandObj, commandPath, &nodeId, &controller)) { @@ -473,58 +548,66 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath __block CommandHandler::Handle handle(commandObj); __block ConcreteCommandPath cachedCommandPath(commandPath.mEndpointId, commandPath.mClusterId, commandPath.mCommandId); - auto completionHandler = ^( - MTROTASoftwareUpdateProviderClusterQueryImageResponseParams * _Nullable data, NSError * _Nullable error) { - dispatch_async(mWorkQueue, ^{ - CommandHandler * handler = EnsureValidState(handle, cachedCommandPath, "QueryImage", data, error); - VerifyOrReturn(handler != nullptr); - - ChipLogDetail(Controller, "QueryImage: application responded with: %s", - [[data description] cStringUsingEncoding:NSUTF8StringEncoding]); - - Commands::QueryImageResponse::Type response; - ConvertFromQueryImageResponseParms(data, response); - - auto hasUpdate = [data.status isEqual:@(MTROTASoftwareUpdateProviderOTAQueryStatusUpdateAvailable)]; - auto isBDXProtocolSupported = - [commandParams.protocolsSupported containsObject:@(MTROTASoftwareUpdateProviderOTADownloadProtocolBDXSynchronous)]; - - if (hasUpdate && isBDXProtocolSupported) { - auto fabricIndex = handler->GetSubjectDescriptor().fabricIndex; - auto nodeId = handler->GetSubjectDescriptor().subject; - CHIP_ERROR err = gOtaSender.PrepareForTransfer(fabricIndex, nodeId); - if (CHIP_NO_ERROR != err) { - LogErrorOnFailure(err); - handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); - handle.Release(); - return; - } - - auto targetNodeId = handler->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetLocalScopedNodeId(); - - char uriBuffer[kMaxBDXURILen]; - MutableCharSpan uri(uriBuffer); - err = bdx::MakeURI(targetNodeId.GetNodeId(), AsCharSpan(data.imageURI), uri); - if (CHIP_NO_ERROR != err) { - LogErrorOnFailure(err); - handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); - handle.Release(); - return; - } - - response.imageURI.SetValue(uri); - handler->AddResponse(cachedCommandPath, response); - handle.Release(); - return; - } - - handler->AddResponse(cachedCommandPath, response); - handle.Release(); - }); - }; + auto completionHandler + = ^(MTROTASoftwareUpdateProviderClusterQueryImageResponseParams * _Nullable data, NSError * _Nullable error) { + [controller + asyncDispatchToMatterQueue:^(chip::Controller::DeviceCommissioner *) { + assertChipStackLockedByCurrentThread(); + + CommandHandler * handler = EnsureValidState(handle, cachedCommandPath, "QueryImage", data, error); + VerifyOrReturn(handler != nullptr); + + ChipLogDetail(Controller, "QueryImage: application responded with: %s", + [[data description] cStringUsingEncoding:NSUTF8StringEncoding]); + + Commands::QueryImageResponse::Type response; + ConvertFromQueryImageResponseParms(data, response); + + auto hasUpdate = [data.status isEqual:@(MTROtaSoftwareUpdateProviderOTAQueryStatusUpdateAvailable)]; + auto isBDXProtocolSupported = [commandParams.protocolsSupported + containsObject:@(MTROtaSoftwareUpdateProviderOTADownloadProtocolBDXSynchronous)]; + + if (hasUpdate && isBDXProtocolSupported) { + auto fabricIndex = handler->GetSubjectDescriptor().fabricIndex; + auto nodeId = handler->GetSubjectDescriptor().subject; + CHIP_ERROR err = gOtaSender.PrepareForTransfer(fabricIndex, nodeId); + if (CHIP_NO_ERROR != err) { + LogErrorOnFailure(err); + handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); + handle.Release(); + return; + } + + auto targetNodeId + = handler->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetLocalScopedNodeId(); + + char uriBuffer[kMaxBDXURILen]; + MutableCharSpan uri(uriBuffer); + err = bdx::MakeURI(targetNodeId.GetNodeId(), AsCharSpan(data.imageURI), uri); + if (CHIP_NO_ERROR != err) { + LogErrorOnFailure(err); + handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Failure); + handle.Release(); + return; + } + + response.imageURI.SetValue(uri); + handler->AddResponse(cachedCommandPath, response); + handle.Release(); + return; + } + + handler->AddResponse(cachedCommandPath, response); + handle.Release(); + } + + errorHandler:^(NSError *) { + // Not much we can do here + }]; + }; auto strongDelegate = mDelegate; - dispatch_async(mWorkQueue, ^{ + dispatch_async(mDelegateNotificationQueue, ^{ if ([strongDelegate respondsToSelector:@selector(handleQueryImageForNodeID:controller:params:completion:)]) { [strongDelegate handleQueryImageForNodeID:@(nodeId) controller:controller @@ -547,6 +630,8 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath void MTROTAProviderDelegateBridge::HandleApplyUpdateRequest(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, const Commands::ApplyUpdateRequest::DecodableType & commandData) { + assertChipStackLockedByCurrentThread(); + NodeId nodeId; MTRDeviceController * controller; if (!GetPeerNodeInfo(commandObj, commandPath, &nodeId, &controller)) { @@ -559,25 +644,31 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath auto completionHandler = ^(MTROTASoftwareUpdateProviderClusterApplyUpdateResponseParams * _Nullable data, NSError * _Nullable error) { - dispatch_async(mWorkQueue, ^{ - CommandHandler * handler = EnsureValidState(handle, cachedCommandPath, "ApplyUpdateRequest", data, error); - VerifyOrReturn(handler != nullptr); - - ChipLogDetail(Controller, "ApplyUpdateRequest: application responded with: %s", - [[data description] cStringUsingEncoding:NSUTF8StringEncoding]); - - Commands::ApplyUpdateResponse::Type response; - ConvertFromApplyUpdateRequestResponseParms(data, response); - handler->AddResponse(cachedCommandPath, response); - handle.Release(); - }); + [controller + asyncDispatchToMatterQueue:^(chip::Controller::DeviceCommissioner *) { + assertChipStackLockedByCurrentThread(); + + CommandHandler * handler = EnsureValidState(handle, cachedCommandPath, "ApplyUpdateRequest", data, error); + VerifyOrReturn(handler != nullptr); + + ChipLogDetail(Controller, "ApplyUpdateRequest: application responded with: %s", + [[data description] cStringUsingEncoding:NSUTF8StringEncoding]); + + Commands::ApplyUpdateResponse::Type response; + ConvertFromApplyUpdateRequestResponseParms(data, response); + handler->AddResponse(cachedCommandPath, response); + handle.Release(); + } + errorHandler:^(NSError *) { + // Not much we can do here + }]; }; auto * commandParams = [[MTROTASoftwareUpdateProviderClusterApplyUpdateRequestParams alloc] init]; ConvertToApplyUpdateRequestParams(commandData, commandParams); auto strongDelegate = mDelegate; - dispatch_async(mWorkQueue, ^{ + dispatch_async(mDelegateNotificationQueue, ^{ if ([strongDelegate respondsToSelector:@selector(handleApplyUpdateRequestForNodeID:controller:params:completion:)]) { [strongDelegate handleApplyUpdateRequestForNodeID:@(nodeId) controller:controller @@ -601,6 +692,8 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath void MTROTAProviderDelegateBridge::HandleNotifyUpdateApplied(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, const Commands::NotifyUpdateApplied::DecodableType & commandData) { + assertChipStackLockedByCurrentThread(); + NodeId nodeId; MTRDeviceController * controller; if (!GetPeerNodeInfo(commandObj, commandPath, &nodeId, &controller)) { @@ -612,20 +705,26 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath __block ConcreteCommandPath cachedCommandPath(commandPath.mEndpointId, commandPath.mClusterId, commandPath.mCommandId); auto completionHandler = ^(NSError * _Nullable error) { - dispatch_async(mWorkQueue, ^{ - CommandHandler * handler = EnsureValidState(handle, cachedCommandPath, "NotifyUpdateApplied", error); - VerifyOrReturn(handler != nullptr); + [controller + asyncDispatchToMatterQueue:^(chip::Controller::DeviceCommissioner *) { + assertChipStackLockedByCurrentThread(); - handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Success); - handle.Release(); - }); + CommandHandler * handler = EnsureValidState(handle, cachedCommandPath, "NotifyUpdateApplied", error); + VerifyOrReturn(handler != nullptr); + + handler->AddStatus(cachedCommandPath, Protocols::InteractionModel::Status::Success); + handle.Release(); + } + errorHandler:^(NSError *) { + // Not much we can do here + }]; }; auto * commandParams = [[MTROTASoftwareUpdateProviderClusterNotifyUpdateAppliedParams alloc] init]; ConvertToNotifyUpdateAppliedParams(commandData, commandParams); auto strongDelegate = mDelegate; - dispatch_async(mWorkQueue, ^{ + dispatch_async(mDelegateNotificationQueue, ^{ if ([strongDelegate respondsToSelector:@selector(handleNotifyUpdateAppliedForNodeID:controller:params:completion:)]) { [strongDelegate handleNotifyUpdateAppliedForNodeID:@(nodeId) controller:controller