From c1d543182fccd0c37c03379602a86fddadff68b1 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 30 Jun 2022 20:58:20 -0400 Subject: [PATCH] Enable "are we on the right thread?" asserts on Darwin. (#20177) A few changes here: 1) Enable chip_stack_lock_tracking on darwin. 2) Implement _IsChipStackLockedByCurrentThread on Darwin in a way that makes sense with the actual "no locks" setup there (by checking that we are on the right dispatch queue). 3) Fix controller shutdown on Darwin to not happen from outside the Matter event loop while the event loop is running. Removing the dealloc method from the Darwin controller implementation is safe because: * The only place where controllers get allocated is MTRControllerFactory's createController. * At that callsite, if initWithFactory returns nil it guarantees that it has called cleanup. * After allocation + init, we add the controller to the factory's controller list. After that the controller cannot be deallocated until we remove it from that list, which only happens in controllerShuttingDown, which is always followed by a call to cleanup. --- .../Framework/CHIP/CHIPDeviceController.mm | 22 ++++++++++--------- .../CHIP/CHIPDeviceController_Internal.h | 8 +++++++ .../Framework/CHIP/MatterControllerFactory.mm | 7 ++++++ src/platform/BUILD.gn | 2 +- src/platform/Darwin/PlatformManagerImpl.cpp | 22 +++++++++++++++---- src/platform/Darwin/PlatformManagerImpl.h | 9 ++++++-- 6 files changed, 53 insertions(+), 17 deletions(-) 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); } };