From 6fd06ea0268b72752b28f4e19221100c9624f6f6 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 1 Jul 2022 15:31:28 -0400 Subject: [PATCH] Fix some threading issues on Darwin. (#20197) * Fix some threading issues on Darwin. There were two places where we were touching SDK data structures from the wrong event queue. * Address review comments --- .../commands/tests/TestCommandBridge.h | 9 +- src/darwin/Framework/CHIP/CHIPDevice.mm | 142 ++++++++++-------- .../Framework/CHIP/CHIPDevice_Internal.h | 7 + 3 files changed, 87 insertions(+), 71 deletions(-) diff --git a/examples/darwin-framework-tool/commands/tests/TestCommandBridge.h b/examples/darwin-framework-tool/commands/tests/TestCommandBridge.h index 4e5c870225ae0e..06479a115a922f 100644 --- a/examples/darwin-framework-tool/commands/tests/TestCommandBridge.h +++ b/examples/darwin-framework-tool/commands/tests/TestCommandBridge.h @@ -117,14 +117,11 @@ class TestCommandBridge : public CHIPCommandBridge, SetIdentity(identity); - // Disconnect our existing device; otherwise getConnectedDevice will - // just hand it right back to us without establishing a new CASE + // Invalidate our existing CASE session; otherwise getConnectedDevice + // will just hand it right back to us without establishing a new CASE // session. if (GetDevice(identity) != nil) { - auto device = [GetDevice(identity) internalDevice]; - if (device != nullptr) { - device->Disconnect(); - } + [GetDevice(identity) invalidateCASESession]; mConnectedDevices[identity] = nil; } diff --git a/src/darwin/Framework/CHIP/CHIPDevice.mm b/src/darwin/Framework/CHIP/CHIPDevice.mm index 0f2c72f3675897..e25a580aa0c714 100644 --- a/src/darwin/Framework/CHIP/CHIPDevice.mm +++ b/src/darwin/Framework/CHIP/CHIPDevice.mm @@ -244,6 +244,16 @@ - (instancetype)initWithDevice:(chip::DeviceProxy *)device return _cppDevice; } +- (void)invalidateCASESession +{ + dispatch_sync(DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ + DeviceProxy * device = [self internalDevice]; + if (device != nullptr) { + device->Disconnect(); + } + }); +} + typedef void (^ReportCallback)(NSArray * _Nullable value, NSError * _Nullable error); typedef void (^DataReportCallback)(NSArray * value); typedef void (^ErrorCallback)(NSError * error); @@ -371,76 +381,78 @@ - (void)subscribeWithQueue:(dispatch_queue_t)queue errorHandler:(void (^)(NSError * error))errorHandler subscriptionEstablished:(nullable void (^)(void))subscriptionEstablishedHandler { - DeviceProxy * device = [self internalDevice]; - if (!device) { - dispatch_async(queue, ^{ - errorHandler([CHIPError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]); - }); - return; - } - - // Wildcard endpoint, cluster, attribute, event. - auto attributePath = std::make_unique(); - auto eventPath = std::make_unique(); - ReadPrepareParams readParams(device->GetSecureSession().Value()); - readParams.mMinIntervalFloorSeconds = minInterval; - readParams.mMaxIntervalCeilingSeconds = maxInterval; - readParams.mpAttributePathParamsList = attributePath.get(); - readParams.mAttributePathParamsListSize = 1; - readParams.mpEventPathParamsList = eventPath.get(); - readParams.mEventPathParamsListSize = 1; - readParams.mKeepSubscriptions - = (params != nil) && (params.keepPreviousSubscriptions != nil) && [params.keepPreviousSubscriptions boolValue]; - - std::unique_ptr callback; - std::unique_ptr readClient; - std::unique_ptr attributeCache; - if (attributeCacheContainer) { - __weak CHIPAttributeCacheContainer * weakPtr = attributeCacheContainer; - callback = std::make_unique( - queue, attributeReportHandler, eventReportHandler, errorHandler, subscriptionEstablishedHandler, ^{ - CHIPAttributeCacheContainer * container = weakPtr; - if (container) { - container.cppAttributeCache = nullptr; - } + dispatch_async(DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{ + DeviceProxy * device = [self internalDevice]; + if (!device) { + dispatch_async(queue, ^{ + errorHandler([CHIPError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]); }); - attributeCache = std::make_unique(*callback.get()); - readClient = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), - attributeCache->GetBufferedCallback(), ReadClient::InteractionType::Subscribe); - } else { - callback = std::make_unique( - queue, attributeReportHandler, eventReportHandler, errorHandler, subscriptionEstablishedHandler); - readClient = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), - callback->GetBufferedCallback(), ReadClient::InteractionType::Subscribe); - } + return; + } - CHIP_ERROR err; - if (params != nil && params.autoResubscribe != nil && ![params.autoResubscribe boolValue]) { - err = readClient->SendRequest(readParams); - } else { - // SendAutoResubscribeRequest cleans up the params, even on failure. - attributePath.release(); - eventPath.release(); - err = readClient->SendAutoResubscribeRequest(std::move(readParams)); - } + // Wildcard endpoint, cluster, attribute, event. + auto attributePath = std::make_unique(); + auto eventPath = std::make_unique(); + ReadPrepareParams readParams(device->GetSecureSession().Value()); + readParams.mMinIntervalFloorSeconds = minInterval; + readParams.mMaxIntervalCeilingSeconds = maxInterval; + readParams.mpAttributePathParamsList = attributePath.get(); + readParams.mAttributePathParamsListSize = 1; + readParams.mpEventPathParamsList = eventPath.get(); + readParams.mEventPathParamsListSize = 1; + readParams.mKeepSubscriptions + = (params != nil) && (params.keepPreviousSubscriptions != nil) && [params.keepPreviousSubscriptions boolValue]; + + std::unique_ptr callback; + std::unique_ptr readClient; + std::unique_ptr attributeCache; + if (attributeCacheContainer) { + __weak CHIPAttributeCacheContainer * weakPtr = attributeCacheContainer; + callback = std::make_unique( + queue, attributeReportHandler, eventReportHandler, errorHandler, subscriptionEstablishedHandler, ^{ + CHIPAttributeCacheContainer * container = weakPtr; + if (container) { + container.cppAttributeCache = nullptr; + } + }); + attributeCache = std::make_unique(*callback.get()); + readClient = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), + attributeCache->GetBufferedCallback(), ReadClient::InteractionType::Subscribe); + } else { + callback = std::make_unique( + queue, attributeReportHandler, eventReportHandler, errorHandler, subscriptionEstablishedHandler); + readClient = std::make_unique(InteractionModelEngine::GetInstance(), device->GetExchangeManager(), + callback->GetBufferedCallback(), ReadClient::InteractionType::Subscribe); + } - if (err != CHIP_NO_ERROR) { - dispatch_async(queue, ^{ - errorHandler([CHIPError errorForCHIPErrorCode:err]); - }); + CHIP_ERROR err; + if (params != nil && params.autoResubscribe != nil && ![params.autoResubscribe boolValue]) { + err = readClient->SendRequest(readParams); + } else { + // SendAutoResubscribeRequest cleans up the params, even on failure. + attributePath.release(); + eventPath.release(); + err = readClient->SendAutoResubscribeRequest(std::move(readParams)); + } - return; - } + if (err != CHIP_NO_ERROR) { + dispatch_async(queue, ^{ + errorHandler([CHIPError errorForCHIPErrorCode:err]); + }); - if (attributeCacheContainer) { - attributeCacheContainer.cppAttributeCache = attributeCache.get(); - // ClusterStateCache will be deleted when OnDone is called or an error is encountered as well. - callback->AdoptAttributeCache(std::move(attributeCache)); - } - // Callback and ReadClient will be deleted when OnDone is called or an error is - // encountered. - callback->AdoptReadClient(std::move(readClient)); - callback.release(); + return; + } + + if (attributeCacheContainer) { + attributeCacheContainer.cppAttributeCache = attributeCache.get(); + // ClusterStateCache will be deleted when OnDone is called or an error is encountered as well. + callback->AdoptAttributeCache(std::move(attributeCache)); + } + // Callback and ReadClient will be deleted when OnDone is called or an error is + // encountered. + callback->AdoptReadClient(std::move(readClient)); + callback.release(); + }); } // Convert TLV data into NSObject diff --git a/src/darwin/Framework/CHIP/CHIPDevice_Internal.h b/src/darwin/Framework/CHIP/CHIPDevice_Internal.h index ffbd98b7a591f1..e8e4ecafa5d0f6 100644 --- a/src/darwin/Framework/CHIP/CHIPDevice_Internal.h +++ b/src/darwin/Framework/CHIP/CHIPDevice_Internal.h @@ -32,6 +32,13 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithDevice:(chip::DeviceProxy *)device; - (chip::DeviceProxy *)internalDevice; +/** + * Invalidate the CASE session, so an attempt to getConnectedDevice for this + * device id will have to create a new CASE session. Ideally this API will go + * away. + */ +- (void)invalidateCASESession; + @end @interface CHIPAttributePath ()