Move more of the MTRAttributeValueWaiter logic into the waiter. (#36416)

* Move more of the MTRAttributeValueWaiter logic into the waiter. This logic should not live in MTRDevice. * Address review comments.
project-chip · Nov 14, 2024 · b8898ab · b8898ab
1 parent 1bdcb74
commit b8898ab
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 65 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRAttributeValueWaiter.mm b/src/darwin/Framework/CHIP/MTRAttributeValueWaiter.mm
@@ -15,12 +15,15 @@
  */
 
 #import <Foundation/Foundation.h>
+#import <os/lock.h>
 
 #import <Matter/MTRError.h>
 
 #import "MTRAttributeValueWaiter_Internal.h"
 #import "MTRDevice_Internal.h"
+#import "MTRError_Internal.h"
 #import "MTRLogging_Internal.h"
+#import "MTRUnfairLock.h"
 
 @implementation MTRAwaitedAttributeState
 - (instancetype)initWithValue:(MTRDeviceDataValueDictionary)value
@@ -40,10 +43,14 @@ @interface MTRAttributeValueWaiter ()
 // Protected by the MTRDevice's lock.
 @property (nonatomic, readwrite, retain) dispatch_queue_t queue;
 @property (nonatomic, readwrite, copy, nullable) MTRStatusCompletion completion;
+@property (nonatomic, retain, readwrite, nullable) dispatch_source_t expirationTimer;
 @property (nonatomic, readonly, retain) MTRDevice * device;
 @end
 
-@implementation MTRAttributeValueWaiter
+@implementation MTRAttributeValueWaiter {
+    // Protects queue/completion and expirationTimer.
+    os_unfair_lock _lock;
+}
 
 - (instancetype)initWithDevice:(MTRDevice *)device values:(NSDictionary<MTRAttributePath *, MTRDeviceDataValueDictionary> *)values queue:(dispatch_queue_t)queue completion:(MTRStatusCompletion)completion
 {
@@ -58,6 +65,7 @@ - (instancetype)initWithDevice:(MTRDevice *)device values:(NSDictionary<MTRAttri
         _completion = completion;
         _device = device;
         _UUID = [NSUUID UUID];
+        _lock = OS_UNFAIR_LOCK_INIT;
     }
 
     return self;
@@ -70,7 +78,13 @@ - (void)dealloc
 
 - (void)cancel
 {
-    [self.device _attributeWaitCanceled:self];
+    [self.device _forgetAttributeWaiter:self];
+    [self _notifyCancellation];
+}
+
+- (void)_notifyCancellation
+{
+    [self _notifyWithError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_CANCELLED]];
 }
 
 - (BOOL)_attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTRAttributePath *)path byDevice:(MTRDevice *)device
@@ -99,15 +113,24 @@ - (BOOL)allValuesSatisfied
 
 - (void)_notifyWithError:(NSError * _Nullable)error
 {
-    // This is always called with the device lock held, so checking and mutating
-    // self.completion here is atomic.
-    if (!self.completion) {
-        return;
-    }
+    MTRStatusCompletion completion;
+    dispatch_queue_t queue;
+    {
+        // Ensure that we only call our completion once.
+        std::lock_guard lock(_lock);
+        if (!self.completion) {
+            return;
+        }
 
-    if (self.expirationTimer != nil) {
-        dispatch_source_cancel(self.expirationTimer);
-        self.expirationTimer = nil;
+        completion = self.completion;
+        queue = self.queue;
+        self.completion = nil;
+        self.queue = nil;
+
+        if (self.expirationTimer != nil) {
+            dispatch_source_cancel(self.expirationTimer);
+            self.expirationTimer = nil;
+        }
     }
 
     if (!error) {
@@ -120,15 +143,41 @@ - (void)_notifyWithError:(NSError * _Nullable)error
         MTR_LOG("%@ %p wait for attribute values unknown error: %@", self, self, error);
     }
 
-    auto completion = self.completion;
-    auto queue = self.queue;
-    self.completion = nil;
-    self.queue = nil;
     dispatch_async(queue, ^{
         completion(error);
     });
 }
 
+- (void)_startTimerWithTimeout:(NSTimeInterval)timeout
+{
+    // Have the timer dispatch on the device queue, so we are not trying to do
+    // it on the API consumer queue (which might be blocked by the API
+    // consumer).
+    dispatch_source_t timerSource = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, self.device.queue);
+
+    // Set a timer to go off after timeout, and not repeat.
+    dispatch_source_set_timer(timerSource, dispatch_time(DISPATCH_TIME_NOW, static_cast<uint64_t>(timeout * static_cast<double>(NSEC_PER_SEC))), DISPATCH_TIME_FOREVER,
+        // Allow .5 seconds of leeway; should be plenty, in practice.
+        static_cast<uint64_t>(0.5 * static_cast<double>(NSEC_PER_SEC)));
+
+    mtr_weakify(self);
+    dispatch_source_set_event_handler(timerSource, ^{
+        dispatch_source_cancel(timerSource);
+        mtr_strongify(self);
+        if (self != nil) {
+            [self.device _forgetAttributeWaiter:self];
+            [self _notifyWithError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_TIMEOUT]];
+        }
+    });
+
+    {
+        std::lock_guard lock(_lock);
+        self.expirationTimer = timerSource;
+    }
+
+    dispatch_resume(timerSource);
+}
+
 - (NSString *)description
 {
     return [NSString stringWithFormat:@"<%@: %@>", NSStringFromClass(self.class), self.UUID];

diff --git a/src/darwin/Framework/CHIP/MTRAttributeValueWaiter_Internal.h b/src/darwin/Framework/CHIP/MTRAttributeValueWaiter_Internal.h
@@ -37,7 +37,6 @@ MTR_DIRECT_MEMBERS
 @interface MTRAttributeValueWaiter ()
 
 @property (nonatomic, readonly) BOOL allValuesSatisfied;
-@property (nonatomic, retain, readwrite, nullable) dispatch_source_t expirationTimer;
 
 - (instancetype)initWithDevice:(MTRDevice *)device values:(NSDictionary<MTRAttributePath *, MTRDeviceDataValueDictionary> *)values queue:(dispatch_queue_t)queue completion:(MTRStatusCompletion)completion;
 
@@ -46,6 +45,15 @@ MTR_DIRECT_MEMBERS
 
 - (void)_notifyWithError:(NSError * _Nullable)error;
 
+// Starts the timer for our timeout, using the device's queue as the dispatch
+// queuue for the timer firing.
+- (void)_startTimerWithTimeout:(NSTimeInterval)timeout;
+
+// Cancels the waiter without trying to remove it from the MTRDevice's
+// collection of waiters (unlike "cancel", which does that removal).  This is
+// exposed so that MTRDevice can do it when invalidating.
+- (void)_notifyCancellation;
+
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -753,7 +753,6 @@ - (MTRAttributeValueWaiter *)waitForAttributeValues:(NSDictionary<MTRAttributePa
 
     NSArray<MTRDeviceResponseValueDictionary> * currentValues = [self readAttributePaths:requestPaths];
 
-    std::lock_guard lock(_lock);
     auto * attributeWaiter = [[MTRAttributeValueWaiter alloc] initWithDevice:self values:values queue:queue completion:completion];
 
     for (MTRDeviceResponseValueDictionary currentValue in currentValues) {
@@ -764,37 +763,23 @@ - (MTRAttributeValueWaiter *)waitForAttributeValues:(NSDictionary<MTRAttributePa
 
     if (attributeWaiter.allValuesSatisfied) {
         MTR_LOG("%@ waitForAttributeValues no need to wait, values already match: %@", self, values);
+        // We haven't added this waiter to self.attributeValueWaiters yet, so
+        // no need to remove it before notifying.
         [attributeWaiter _notifyWithError:nil];
         return attributeWaiter;
     }
 
-    // Otherwise, wait for the values to arrive or our timeout.
-    if (!self.attributeValueWaiters) {
-        self.attributeValueWaiters = [NSHashTable weakObjectsHashTable];
+    // Otherwise, wait for one of our termination conditions.
+    {
+        std::lock_guard lock(_lock);
+        if (!self.attributeValueWaiters) {
+            self.attributeValueWaiters = [NSHashTable weakObjectsHashTable];
+        }
+        [self.attributeValueWaiters addObject:attributeWaiter];
     }
-    [self.attributeValueWaiters addObject:attributeWaiter];
 
     MTR_LOG("%@ waitForAttributeValues will wait up to %f seconds for %@", self, timeout, values);
-    dispatch_source_t timerSource = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, self.queue);
-    attributeWaiter.expirationTimer = timerSource;
-
-    // Set a timer to go off after timeout, and not repeat.
-    dispatch_source_set_timer(timerSource, dispatch_time(DISPATCH_TIME_NOW, static_cast<uint64_t>(timeout * static_cast<double>(NSEC_PER_SEC))), DISPATCH_TIME_FOREVER,
-        // Allow .5 seconds of leeway; should be plenty, in practice.
-        static_cast<uint64_t>(0.5 * static_cast<double>(NSEC_PER_SEC)));
-
-    mtr_weakify(attributeWaiter);
-    mtr_weakify(self);
-    dispatch_source_set_event_handler(timerSource, ^{
-        dispatch_source_cancel(timerSource);
-        mtr_strongify(self);
-        mtr_strongify(attributeWaiter);
-        if (self != nil && attributeWaiter != nil) {
-            [self _attributeWaitTimedOut:attributeWaiter];
-        }
-    });
-
-    dispatch_resume(timerSource);
+    [attributeWaiter _startTimerWithTimeout:timeout];
     return attributeWaiter;
 }
 
@@ -814,35 +799,15 @@ - (void)_attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTR
     }
 
     for (MTRAttributeValueWaiter * attributeValueWaiter in satisfiedWaiters) {
-        [self _notifyAttributeValueWaiter:attributeValueWaiter withError:nil];
+        [self.attributeValueWaiters removeObject:attributeValueWaiter];
+        [attributeValueWaiter _notifyWithError:nil];
     }
 }
 
-- (void)_attributeWaitTimedOut:(MTRAttributeValueWaiter *)attributeValueWaiter
+- (void)_forgetAttributeWaiter:(MTRAttributeValueWaiter *)attributeValueWaiter
 {
     std::lock_guard lock(_lock);
-    [self _notifyAttributeValueWaiter:attributeValueWaiter withError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_TIMEOUT]];
-}
-
-- (void)_attributeWaitCanceled:(MTRAttributeValueWaiter *)attributeValueWaiter
-{
-    std::lock_guard lock(_lock);
-    [self _doAttributeWaitCanceled:attributeValueWaiter];
-}
-
-- (void)_doAttributeWaitCanceled:(MTRAttributeValueWaiter *)attributeValueWaiter
-{
-    os_unfair_lock_assert_owner(&_lock);
-
-    [self _notifyAttributeValueWaiter:attributeValueWaiter withError:[MTRError errorForCHIPErrorCode:CHIP_ERROR_CANCELLED]];
-}
-
-- (void)_notifyAttributeValueWaiter:(MTRAttributeValueWaiter *)attributeValueWaiter withError:(NSError * _Nullable)error
-{
-    os_unfair_lock_assert_owner(&_lock);
-
     [self.attributeValueWaiters removeObject:attributeValueWaiter];
-    [attributeValueWaiter _notifyWithError:error];
 }
 
 - (void)_cancelAllAttributeValueWaiters
@@ -852,7 +817,7 @@ - (void)_cancelAllAttributeValueWaiters
     auto * attributeValueWaiters = self.attributeValueWaiters;
     self.attributeValueWaiters = nil;
     for (MTRAttributeValueWaiter * attributeValueWaiter in attributeValueWaiters) {
-        [self _doAttributeWaitCanceled:attributeValueWaiter];
+        [attributeValueWaiter _notifyCancellation];
     }
 }
 

diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h
@@ -176,7 +176,7 @@ MTR_DIRECT_MEMBERS
 // expected value for the relevant attribute.
 - (void)_attributeValue:(MTRDeviceDataValueDictionary)value reportedForPath:(MTRAttributePath *)path;
 
-- (void)_attributeWaitCanceled:(MTRAttributeValueWaiter *)attributeValueWaiter;
+- (void)_forgetAttributeWaiter:(MTRAttributeValueWaiter *)attributeValueWaiter;
 
 @end