diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm index ba2e4309fb35ce..1521d7a4035f11 100644 --- a/src/darwin/Framework/CHIP/CHIPDeviceController.mm +++ b/src/darwin/Framework/CHIP/CHIPDeviceController.mm @@ -125,14 +125,21 @@ - (void)cleanupAfterStartup [self cleanup]; } +// Part of cleanupAfterStartup that has to interact with the Matter work queue +// in a very specific way that only MTRControllerFactory knows about. +- (void)shutDownCppController +{ + if (_cppCommissioner) { + _cppCommissioner->Shutdown(); + delete _cppCommissioner; + _cppCommissioner = nullptr; + } +} + // Clean up any members we might have allocated. - (void)cleanup { - if (self->_cppCommissioner) { - self->_cppCommissioner->Shutdown(); - delete self->_cppCommissioner; - self->_cppCommissioner = nullptr; - } + VerifyOrDie(_cppCommissioner == nullptr); [self clearDeviceAttestationDelegateBridge]; @@ -675,11 +682,6 @@ - (BOOL)checkForError:(CHIP_ERROR)errorCode logMsg:(NSString *)logMsg error:(NSE return YES; } -- (void)dealloc -{ - [self cleanup]; -} - - (BOOL)deviceBeingCommissionedOverBLE:(uint64_t)deviceId { CHIP_ERROR errorCode = CHIP_ERROR_INCORRECT_STATE; diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController_Internal.h b/src/darwin/Framework/CHIP/CHIPDeviceController_Internal.h index 8419de759b82f6..3c4cd4f0627f4e 100644 --- a/src/darwin/Framework/CHIP/CHIPDeviceController_Internal.h +++ b/src/darwin/Framework/CHIP/CHIPDeviceController_Internal.h @@ -80,6 +80,14 @@ NS_ASSUME_NONNULL_BEGIN fabricIndex:(chip::FabricIndex)fabricIndex isRunning:(BOOL *)isRunning; +/** + * Shut down the underlying C++ controller. Must be called on the Matter work + * queue or after the Matter work queue has been shut down. + * + * Only MTRControllerFactory should be calling this. + */ +- (void)shutDownCppController; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MatterControllerFactory.mm b/src/darwin/Framework/CHIP/MatterControllerFactory.mm index ba8618302b8b57..12db00bf231f9c 100644 --- a/src/darwin/Framework/CHIP/MatterControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MatterControllerFactory.mm @@ -525,6 +525,13 @@ - (void)controllerShuttingDown:(CHIPDeviceController *)controller // shuts down, because shutdown of the last controller will tear // down most of the world. DeviceLayer::PlatformMgrImpl().StopEventLoopTask(); + + [controller shutDownCppController]; + } else { + // Do the controller shutdown on the Matter work queue. + dispatch_sync(_chipWorkQueue, ^{ + [controller shutDownCppController]; + }); } } diff --git a/src/platform/BUILD.gn b/src/platform/BUILD.gn index fdcc45d0424ac7..580bd138ae312b 100644 --- a/src/platform/BUILD.gn +++ b/src/platform/BUILD.gn @@ -73,7 +73,7 @@ if (chip_device_platform != "none" && chip_device_platform != "external") { if (chip_stack_lock_tracking == "auto") { if (chip_device_platform == "linux" || chip_device_platform == "tizen" || chip_device_platform == "android" || current_os == "freertos" || - chip_device_platform == "webos") { + chip_device_platform == "webos" || chip_device_platform == "darwin") { # TODO: should be fatal for development. Change once bugs are fixed chip_stack_lock_tracking = "fatal" } else { diff --git a/src/platform/Darwin/PlatformManagerImpl.cpp b/src/platform/Darwin/PlatformManagerImpl.cpp index 3c85bee76cbd62..1a80645a0192c1 100644 --- a/src/platform/Darwin/PlatformManagerImpl.cpp +++ b/src/platform/Darwin/PlatformManagerImpl.cpp @@ -69,9 +69,9 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() { - if (mIsWorkQueueRunning == false) + if (mIsWorkQueueSuspended) { - mIsWorkQueueRunning = true; + mIsWorkQueueSuspended = false; dispatch_resume(mWorkQueue); } @@ -80,9 +80,9 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask() { - if (mIsWorkQueueRunning == true) + if (!mIsWorkQueueSuspended && !mIsWorkQueueSuspensionPending) { - mIsWorkQueueRunning = false; + mIsWorkQueueSuspensionPending = true; if (dispatch_get_current_queue() != mWorkQueue) { // dispatch_sync is used in order to guarantee serialization of the caller with @@ -90,6 +90,9 @@ CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask() dispatch_sync(mWorkQueue, ^{ dispatch_suspend(mWorkQueue); }); + + mIsWorkQueueSuspended = true; + mIsWorkQueueSuspensionPending = false; } else { @@ -99,6 +102,8 @@ CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask() // that no more tasks will run on the queue. dispatch_async(mWorkQueue, ^{ dispatch_suspend(mWorkQueue); + mIsWorkQueueSuspended = true; + mIsWorkQueueSuspensionPending = false; dispatch_semaphore_signal(mRunLoopSem); }); } @@ -138,5 +143,14 @@ CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event) return CHIP_NO_ERROR; } +#if CHIP_STACK_LOCK_TRACKING_ENABLED +bool PlatformManagerImpl::_IsChipStackLockedByCurrentThread() const +{ + // If we have no work queue, or it's suspended, then we assume our caller + // knows what they are doing in terms of their own concurrency. + return !mWorkQueue || mIsWorkQueueSuspended || dispatch_get_current_queue() == mWorkQueue; +}; +#endif + } // namespace DeviceLayer } // namespace chip diff --git a/src/platform/Darwin/PlatformManagerImpl.h b/src/platform/Darwin/PlatformManagerImpl.h index 72b0bd9d7103d2..e8b8decf2fcfb3 100644 --- a/src/platform/Darwin/PlatformManagerImpl.h +++ b/src/platform/Darwin/PlatformManagerImpl.h @@ -49,6 +49,7 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener { mWorkQueue = dispatch_queue_create(CHIP_CONTROLLER_QUEUE, DISPATCH_QUEUE_SERIAL); dispatch_suspend(mWorkQueue); + mIsWorkQueueSuspended = true; } return mWorkQueue; } @@ -71,7 +72,7 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener CHIP_ERROR _PostEvent(const ChipDeviceEvent * event); #if CHIP_STACK_LOCK_TRACKING_ENABLED - bool _IsChipStackLockedByCurrentThread() const { return false; }; + bool _IsChipStackLockedByCurrentThread() const; #endif // ===== Members for internal use by the following friends. @@ -88,7 +89,11 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener // Semaphore used to implement blocking behavior in _RunEventLoop. dispatch_semaphore_t mRunLoopSem; - bool mIsWorkQueueRunning = false; + bool mIsWorkQueueSuspended = false; + // TODO: mIsWorkQueueSuspensionPending might need to be an atomic and use + // atomic ops, if we're worried about calls to StopEventLoopTask() from + // multiple threads racing somehow... + bool mIsWorkQueueSuspensionPending = false; inline ImplClass * Impl() { return static_cast(this); } };