From f4f1e19b0d9dda19e1fc972ed14331c4f6c07553 Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Tue, 19 Mar 2024 11:20:22 -0700 Subject: [PATCH] Moving more Darwin stuff to std::lock_guard lock(_lock); (#32558) * Initial commit * Restyled by whitespace * Reverting this one * Addressing feedback * Adding a few more * Update src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm * Removing this one * Restyled by clang-format * Reverting these --------- Co-authored-by: Restyled.io --- .../CHIP/MTRAsyncCallbackWorkQueue.mm | 30 +++---- .../Framework/CHIP/MTRAsyncWorkQueue.mm | 14 ++-- src/darwin/Framework/CHIP/MTRDevice.mm | 81 ++++++------------- .../Framework/CHIP/MTRDeviceController.mm | 7 +- .../CHIP/MTRDeviceControllerFactory.mm | 9 +-- src/darwin/Framework/CHIP/MTRLogging.mm | 4 +- 6 files changed, 48 insertions(+), 97 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm index 00b6d33788f6c0..1e44b4a04ef702 100644 --- a/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm +++ b/src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm @@ -23,6 +23,8 @@ #import #import "MTRLogging_Internal.h" +#import "MTRUnfairLock.h" + #import #pragma mark - Class extensions @@ -72,14 +74,10 @@ - (instancetype)initWithContext:(id)context queue:(dispatch_queue_t)queue - (NSString *)description { - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); - auto * desc = [NSString + return [NSString stringWithFormat:@"MTRAsyncCallbackWorkQueue context: %@ items count: %lu", self.context, (unsigned long) self.items.count]; - - os_unfair_lock_unlock(&_lock); - - return desc; } - (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item @@ -91,12 +89,11 @@ - (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item [item markEnqueued]; - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); item.workQueue = self; [self.items addObject:item]; [self _callNextReadyWorkItem]; - os_unfair_lock_unlock(&_lock); } - (void)invalidate @@ -115,11 +112,10 @@ - (void)invalidate // called after executing a work item - (void)_postProcessWorkItem:(MTRAsyncCallbackQueueWorkItem *)workItem retry:(BOOL)retry { - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); // sanity check if running if (!self.runningWorkItemCount) { // something is wrong with state - nothing is currently running - os_unfair_lock_unlock(&_lock); MTR_LOG_ERROR("MTRAsyncCallbackWorkQueue endWork: no work is running on work queue"); return; } @@ -129,7 +125,6 @@ - (void)_postProcessWorkItem:(MTRAsyncCallbackQueueWorkItem *)workItem retry:(BO MTRAsyncCallbackQueueWorkItem * firstWorkItem = self.items.firstObject; if (firstWorkItem != workItem) { // something is wrong with this work item - should not be currently running - os_unfair_lock_unlock(&_lock); MTR_LOG_ERROR("MTRAsyncCallbackWorkQueue endWork: work item is not first on work queue"); return; } @@ -142,7 +137,6 @@ - (void)_postProcessWorkItem:(MTRAsyncCallbackQueueWorkItem *)workItem retry:(BO // when "concurrency width" is implemented this will be decremented instead self.runningWorkItemCount = 0; [self _callNextReadyWorkItem]; - os_unfair_lock_unlock(&_lock); } - (void)endWork:(MTRAsyncCallbackQueueWorkItem *)workItem @@ -203,34 +197,30 @@ - (void)_invalidate - (void)invalidate { - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); [self _invalidate]; - os_unfair_lock_unlock(&_lock); } - (void)markEnqueued { - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); _enqueued = YES; - os_unfair_lock_unlock(&_lock); } - (void)setReadyHandler:(MTRAsyncCallbackReadyHandler)readyHandler { - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); if (!_enqueued) { _readyHandler = readyHandler; } - os_unfair_lock_unlock(&_lock); } - (void)setCancelHandler:(dispatch_block_t)cancelHandler { - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); if (!_enqueued) { _cancelHandler = cancelHandler; } - os_unfair_lock_unlock(&_lock); } - (void)endWork diff --git a/src/darwin/Framework/CHIP/MTRAsyncWorkQueue.mm b/src/darwin/Framework/CHIP/MTRAsyncWorkQueue.mm index ff7242c3582377..be839561c1be33 100644 --- a/src/darwin/Framework/CHIP/MTRAsyncWorkQueue.mm +++ b/src/darwin/Framework/CHIP/MTRAsyncWorkQueue.mm @@ -18,6 +18,7 @@ #import "MTRAsyncWorkQueue.h" #import "MTRDefines_Internal.h" #import "MTRLogging_Internal.h" +#import "MTRUnfairLock.h" #import #import @@ -273,13 +274,12 @@ - (void)enqueueWorkItem:(MTRAsyncWorkItem *)item - (void)invalidate { ContextSnapshot context(_context); // outside of lock - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); MTR_LOG_INFO("MTRAsyncWorkQueue<%@> invalidate %tu items", context.description, _items.count); for (MTRAsyncWorkItem * item in _items) { [item cancel]; } [_items removeAllObjects]; - os_unfair_lock_unlock(&_lock); } - (void)_postProcessWorkItem:(MTRAsyncWorkItem *)workItem @@ -376,8 +376,7 @@ - (void)_callNextReadyWorkItemWithContext:(ContextSnapshot const &)context - (BOOL)hasDuplicateForTypeID:(NSUInteger)opaqueDuplicateTypeID workItemData:(id)opaqueWorkItemData { - BOOL hasDuplicate = NO; - os_unfair_lock_lock(&_lock); + std::lock_guard lock(_lock); // Start from the last item for (MTRAsyncWorkItem * item in [_items reverseObjectEnumerator]) { auto duplicateCheckHandler = item.duplicateCheckHandler; @@ -386,13 +385,12 @@ - (BOOL)hasDuplicateForTypeID:(NSUInteger)opaqueDuplicateTypeID workItemData:(id BOOL isDuplicate = NO; duplicateCheckHandler(opaqueWorkItemData, &isDuplicate, &stop); if (stop) { - hasDuplicate = isDuplicate; - break; + return isDuplicate; } } } - os_unfair_lock_unlock(&_lock); - return hasDuplicate; + + return NO; } @end diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 98387bfde797fb..a63902f68d3711 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -352,22 +352,19 @@ - (void)_updateDeviceTimeAndScheduleNextUpdate - (void)_performScheduledTimeUpdate { - os_unfair_lock_lock(&self->_timeSyncLock); + std::lock_guard lock(_timeSyncLock); // Device needs to still be reachable if (self.state != MTRDeviceStateReachable) { MTR_LOG_DEBUG("%@ Device is not reachable, canceling Device Time Updates.", self); - os_unfair_lock_unlock(&self->_timeSyncLock); return; } // Device must not be invalidated if (!self.timeUpdateScheduled) { MTR_LOG_DEBUG("%@ Device Time Update is no longer scheduled, MTRDevice may have been invalidated.", self); - os_unfair_lock_unlock(&self->_timeSyncLock); return; } self.timeUpdateScheduled = NO; [self _updateDeviceTimeAndScheduleNextUpdate]; - os_unfair_lock_unlock(&self->_timeSyncLock); } - (NSArray *)_endpointsWithTimeSyncClusterServer @@ -504,7 +501,7 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu } #endif - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); _weakDelegate = [MTRWeakReference weakReferenceWithObject:delegate]; _delegateQueue = queue; @@ -517,8 +514,6 @@ - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queu if (setUpSubscription) { [self _setupSubscription]; } - - os_unfair_lock_unlock(&self->_lock); } - (void)invalidate @@ -579,11 +574,9 @@ - (void)nodeMayBeAdvertisingOperational // Return YES if there's a valid delegate AND subscription is expected to report value - (BOOL)_subscriptionAbleToReport { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); id delegate = _weakDelegate.strongObject; - auto state = _state; - os_unfair_lock_unlock(&self->_lock); - return (delegate != nil) && (state == MTRDeviceStateReachable); + return (delegate != nil) && (_state == MTRDeviceStateReachable); } - (BOOL)_callDelegateWithBlock:(void (^)(id))block @@ -667,41 +660,35 @@ - (void)_handleSubscriptionEstablished - (void)_handleSubscriptionError:(NSError *)error { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); _internalDeviceState = MTRInternalDeviceStateUnsubscribed; _unreportedEvents = nil; [self _changeState:MTRDeviceStateUnreachable]; - - os_unfair_lock_unlock(&self->_lock); } - (void)_handleResubscriptionNeeded { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); [self _changeState:MTRDeviceStateUnknown]; - - os_unfair_lock_unlock(&self->_lock); } - (void)_handleSubscriptionReset { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); // if there is no delegate then also do not retry id delegate = _weakDelegate.strongObject; if (!delegate) { // NOTE: Do not log anythig here: we have been invalidated, and the // Matter stack might already be torn down. - os_unfair_lock_unlock(&self->_lock); return; } // don't schedule multiple retries if (self.reattemptingSubscription) { MTR_LOG_DEFAULT("%@ already reattempting subscription", self); - os_unfair_lock_unlock(&self->_lock); return; } @@ -722,8 +709,6 @@ - (void)_handleSubscriptionReset [self _reattemptSubscriptionNowIfNeeded]; os_unfair_lock_unlock(&self->_lock); }); - - os_unfair_lock_unlock(&self->_lock); } - (void)_reattemptSubscriptionNowIfNeeded @@ -740,7 +725,7 @@ - (void)_reattemptSubscriptionNowIfNeeded - (void)_handleUnsolicitedMessageFromPublisher { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); [self _changeState:MTRDeviceStateReachable]; @@ -759,8 +744,6 @@ - (void)_handleUnsolicitedMessageFromPublisher // ReadClient how did we get this notification and if we _do_ have an active // ReadClient, this call or _setupSubscription would be no-ops. [self _reattemptSubscriptionNowIfNeeded]; - - os_unfair_lock_unlock(&self->_lock); } - (void)_markDeviceAsUnreachableIfNotSusbcribed @@ -776,7 +759,8 @@ - (void)_markDeviceAsUnreachableIfNotSusbcribed - (void)_handleReportBegin { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); + _receivingReport = YES; if (_state != MTRDeviceStateReachable) { _receivingPrimingReport = YES; @@ -784,12 +768,11 @@ - (void)_handleReportBegin } else { _receivingPrimingReport = NO; } - os_unfair_lock_unlock(&self->_lock); } - (void)_handleReportEnd { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); _receivingReport = NO; _receivingPrimingReport = NO; _estimatedStartTimeFromGeneralDiagnosticsUpTime = nil; @@ -805,7 +788,6 @@ - (void)_handleReportEnd }); } #endif - os_unfair_lock_unlock(&self->_lock); } // assume lock is held @@ -824,12 +806,10 @@ - (void)_reportAttributes:(NSArray *> *)attributes - (void)_handleAttributeReport:(NSArray *> *)attributeReport { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); // _getAttributesToReportWithReportedValues will log attribute paths reported [self _reportAttributes:[self _getAttributesToReportWithReportedValues:attributeReport]]; - - os_unfair_lock_unlock(&self->_lock); } #ifdef DEBUG @@ -843,7 +823,7 @@ - (void)unitTestInjectEventReport:(NSArray *> *)eve - (void)_handleEventReport:(NSArray *> *)eventReport { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); NSDate * oldEstimatedStartTime = _estimatedStartTime; // Combine with previous unreported events, if they exist @@ -937,14 +917,13 @@ - (void)_handleEventReport:(NSArray *> *)eventRepor // save unreported events _unreportedEvents = reportToReturn; } - - os_unfair_lock_unlock(&self->_lock); } - (NSDictionary *)_getCachedDataVersions { NSMutableDictionary * dataVersions = [NSMutableDictionary dictionary]; - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); + for (MTRAttributePath * path in _readCache) { NSDictionary * dataValue = _readCache[path]; NSNumber * dataVersionNumber = dataValue[MTRDataVersionKey]; @@ -958,7 +937,6 @@ - (void)_handleEventReport:(NSArray *> *)eventRepor } } MTR_LOG_INFO("%@ _getCachedDataVersions dataVersions count: %lu readCache count: %lu", self, static_cast(dataVersions.count), static_cast(_readCache.count)); - os_unfair_lock_unlock(&self->_lock); return dataVersions; } @@ -1083,11 +1061,12 @@ - (void)_setupSubscription os_unfair_lock_lock(&self->_lock); self->_currentReadClient = nullptr; self->_currentSubscriptionCallback = nullptr; - os_unfair_lock_unlock(&self->_lock); + dispatch_async(self.queue, ^{ // OnDone [self _handleSubscriptionReset]; }); + os_unfair_lock_unlock(&self->_lock); }, ^(void) { MTR_LOG_DEFAULT("%@ got unsolicited message from publisher", self); @@ -1855,18 +1834,16 @@ - (void)_checkExpiredExpectedValues - (void)_performScheduledExpirationCheck { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); self.expirationCheckScheduled = NO; [self _checkExpiredExpectedValues]; - - os_unfair_lock_unlock(&self->_lock); } // Get attribute value dictionary for an attribute path from the right cache - (NSDictionary *)_attributeValueDictionaryForAttributePath:(MTRAttributePath *)attributePath { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); // First check expected value cache NSArray * expectedValue = _expectedValueCache[attributePath]; @@ -1876,8 +1853,6 @@ - (void)_performScheduledExpirationCheck // expired - purge and fall through _expectedValueCache[attributePath] = nil; } else { - os_unfair_lock_unlock(&self->_lock); - // not yet expired - return result return expectedValue[MTRDeviceExpectedValueFieldValueIndex]; } @@ -1886,8 +1861,6 @@ - (void)_performScheduledExpirationCheck // Then check read cache NSDictionary * cachedAttributeValue = _readCache[attributePath]; if (cachedAttributeValue) { - os_unfair_lock_unlock(&self->_lock); - return cachedAttributeValue; } else { // TODO: when not found in cache, generated default values should be used @@ -1895,8 +1868,6 @@ - (void)_performScheduledExpirationCheck attributePath); } - os_unfair_lock_unlock(&self->_lock); - return nil; } @@ -2024,7 +1995,7 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray *)attributeValues reportChanges:(BOOL)reportChanges { MTR_LOG_INFO("%@ setAttributeValues count: %lu reportChanges: %d", self, static_cast(attributeValues.count), reportChanges); - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); if (reportChanges) { [self _reportAttributes:[self _getAttributesToReportWithReportedValues:attributeValues]]; @@ -2040,8 +2011,6 @@ - (void)setAttributeValues:(NSArray *)attributeValues reportChan if ([self _isCachePrimedWithInitialConfigurationData]) { _delegateDeviceCachePrimedCalled = YES; } - - os_unfair_lock_unlock(&self->_lock); } - (BOOL)deviceCachePrimed @@ -2166,7 +2135,7 @@ - (void)setExpectedValues:(NSArray *> *)values MTR_LOG_INFO( "%@ Setting expected values %@ with expiration time %f seconds from now", self, values, [expirationTime timeIntervalSinceNow]); - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); // _getAttributesToReportWithNewExpectedValues will log attribute paths reported NSArray * attributesToReport = [self _getAttributesToReportWithNewExpectedValues:values @@ -2175,24 +2144,22 @@ - (void)setExpectedValues:(NSArray *> *)values [self _reportAttributes:attributesToReport]; [self _checkExpiredExpectedValues]; - os_unfair_lock_unlock(&self->_lock); } - (void)removeExpectedValuesForAttributePaths:(NSArray *)attributePaths expectedValueID:(uint64_t)expectedValueID { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); + for (MTRAttributePath * attributePath in attributePaths) { [self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; } - os_unfair_lock_unlock(&self->_lock); } - (void)removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID { - os_unfair_lock_lock(&self->_lock); + std::lock_guard lock(_lock); [self _removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; - os_unfair_lock_unlock(&self->_lock); } - (void)_removeExpectedValueForAttributePath:(MTRAttributePath *)attributePath expectedValueID:(uint64_t)expectedValueID diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 7b7fd457447590..ae6669cdab5add 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -46,6 +46,7 @@ #import "MTRServerEndpoint_Internal.h" #import "MTRSetupPayload.h" #import "MTRTimeUtils.h" +#import "MTRUnfairLock.h" #import "NSDataSpanConversion.h" #import "NSStringSpanConversion.h" #import @@ -916,7 +917,7 @@ - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID { - os_unfair_lock_lock(&_deviceMapLock); + std::lock_guard lock(_deviceMapLock); MTRDevice * deviceToReturn = _nodeIDToDeviceMap[nodeID]; if (!deviceToReturn) { deviceToReturn = [[MTRDevice alloc] initWithNodeID:nodeID controller:self]; @@ -935,14 +936,13 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID [deviceToReturn setAttributeValues:attributesFromCache reportChanges:NO]; } } - os_unfair_lock_unlock(&_deviceMapLock); return deviceToReturn; } - (void)removeDevice:(MTRDevice *)device { - os_unfair_lock_lock(&_deviceMapLock); + std::lock_guard lock(_deviceMapLock); auto * nodeID = device.nodeID; MTRDevice * deviceToRemove = _nodeIDToDeviceMap[nodeID]; if (deviceToRemove == device) { @@ -951,7 +951,6 @@ - (void)removeDevice:(MTRDevice *)device } else { MTR_LOG_ERROR("Error: Cannot remove device %p with nodeID %llu", device, nodeID.unsignedLongLongValue); } - os_unfair_lock_unlock(&_deviceMapLock); } - (void)setDeviceControllerDelegate:(id)delegate queue:(dispatch_queue_t)queue diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm index 2ccf89634646e8..3d84a8fea22571 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm @@ -1121,10 +1121,8 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller - (NSArray *)getRunningControllers { - os_unfair_lock_lock(&_controllersLock); - NSArray * controllersCopy = [_controllers copy]; - os_unfair_lock_unlock(&_controllersLock); - return controllersCopy; + std::lock_guard lock(_controllersLock); + return [_controllers copy]; } - (nullable MTRDeviceController *)runningControllerForFabricIndex:(FabricIndex)fabricIndex @@ -1196,9 +1194,8 @@ - (BOOL)addServerEndpoint:(MTRServerEndpoint *)endpoint - (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint { - os_unfair_lock_lock(&_serverEndpointsLock); + std::lock_guard lock(_serverEndpointsLock); [_serverEndpoints removeObject:endpoint]; - os_unfair_lock_unlock(&_serverEndpointsLock); } - (NSArray *)accessGrantsForFabricIndex:(chip::FabricIndex)fabricIndex clusterPath:(MTRClusterPath *)clusterPath diff --git a/src/darwin/Framework/CHIP/MTRLogging.mm b/src/darwin/Framework/CHIP/MTRLogging.mm index 23b310a37a05fa..e5529780b73f9c 100644 --- a/src/darwin/Framework/CHIP/MTRLogging.mm +++ b/src/darwin/Framework/CHIP/MTRLogging.mm @@ -19,6 +19,7 @@ #import "MTRLogging_Internal.h" #import "MTRFramework.h" +#import "MTRUnfairLock.h" #import #import @@ -56,7 +57,7 @@ void MTRSetLogCallback(MTRLogType logTypeThreshold, MTRLogCallback _Nullable cal { MTRFrameworkInit(); - os_unfair_lock_lock(&logCallbackLock); + std::lock_guard lock(logCallbackLock); if (callback) { SetLogRedirectCallback(&MTRLogCallbackTrampoline); SetLogFilter(static_cast(std::min(std::max(logTypeThreshold, MTRLogTypeError), MTRLogTypeDetail))); @@ -66,5 +67,4 @@ void MTRSetLogCallback(MTRLogType logTypeThreshold, MTRLogCallback _Nullable cal SetLogFilter(kLogCategory_None); SetLogRedirectCallback(nullptr); } - os_unfair_lock_unlock(&logCallbackLock); }