Address review comments

src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm

@@ -47,7 +47,7 @@ @interface MTRAsyncCallbackQueueWorkItem ()
 @property (nonatomic, strong) MTRAsyncCallbackWorkQueue * workQueue;
 @property (nonatomic, readonly) BOOL enqueued;
 // Called by the queue
-- (void)markedEnqueued;
+- (void)markEnqueued;
 - (void)callReadyHandlerWithContext:(id)context;
 - (void)cancel;
 @end
@@ -85,7 +85,7 @@ - (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item
         return;
     }
 
-    [item markedEnqueued];
+    [item markEnqueued];
 
     os_unfair_lock_lock(&_lock);
     item.workQueue = self;
@@ -204,7 +204,7 @@ - (void)invalidate
     os_unfair_lock_unlock(&_lock);
 }
 
-- (void)markedEnqueued
+- (void)markEnqueued
 {
     os_unfair_lock_lock(&_lock);
     _enqueued = YES;

src/darwin/Framework/CHIP/MTRAsyncWorkQueue.h

@@ -33,8 +33,8 @@ typedef BOOL (^MTRAsyncWorkCompletionBlock)(MTRAsyncWorkOutcome outcome);
 /// An optional handler that controls batching of MTRAsyncWorkItem.
 ///
 /// When a work item is dequeued to run, if it is of a type that can be
-/// combined with similar work items in a batch, this facility gives the
-/// provides an opportunity to coalesce and merge work items.
+/// combined with similar work items in a batch, this facility provides an
+/// opportunity to coalesce and merge work items.
 ///
 /// The batching handler is called by the work queue when all of the following
 /// are true:
@@ -84,9 +84,9 @@ typedef void (^MTRAsyncWorkDuplicateCheckHandler)(id opaqueItemData, BOOL * isDu
 /// async work queue in various situations. Generally work items will have at
 /// least a `readyHandler` (though it is technically optional).
 ///
-/// This class is not thread-safe, and once a work item has be submitted to the
-/// queue via `enqueueWorkItem` ownership of the work item passes to the queue.
-/// No further modifications may be made to it after that point.
+/// This class is not thread-safe, and once a work item has been submitted to
+/// the queue via `enqueueWorkItem` ownership of the work item passes to the
+/// queue. No further modifications may be made to it after that point.
 ///
 /// @see -[MTRAsyncWorkQueue enqueueWorkItem:]
 MTR_TESTABLE
@@ -120,8 +120,8 @@ MTR_TESTABLE
 @property (nonatomic, strong, nullable) void (^cancelHandler)(void);
 
 @property (nonatomic, readonly) NSUInteger batchingID;
-@property (nonatomic, readonly) id batchableData;
-@property (nonatomic, readonly) MTRAsyncWorkBatchingHandler batchingHandler;
+@property (nonatomic, readonly, nullable) id batchableData;
+@property (nonatomic, readonly, nullable) MTRAsyncWorkBatchingHandler batchingHandler;
 
 /// Sets the batching handler and associated data for this work item.
 ///
@@ -134,7 +134,7 @@ MTR_TESTABLE
               handler:(MTRAsyncWorkBatchingHandler)batchingHandler;
 
 @property (nonatomic, readonly) NSUInteger duplicateTypeID;
-@property (nonatomic, readonly) MTRAsyncWorkDuplicateCheckHandler duplicateCheckHandler;
+@property (nonatomic, readonly, nullable) MTRAsyncWorkDuplicateCheckHandler duplicateCheckHandler;
 
 /// Sets the duplicate check type and handler for this work item.
 ///

src/darwin/Framework/CHIP/MTRAsyncWorkQueue.mm

@@ -35,6 +35,8 @@ @implementation MTRAsyncWorkItem {
     MTRAsyncWorkItemState _state; // protected by queue lock once enqueued
 }
 
+#pragma mark Configuration by the client
+
 - (instancetype)initWithQueue:(dispatch_queue_t)queue
 {
     NSParameterAssert(queue);
@@ -45,11 +47,6 @@ - (instancetype)initWithQueue:(dispatch_queue_t)queue
     return self;
 }
 
-- (void)assertMutable
-{
-    NSAssert(_state == MTRAsyncWorkItemMutable, @"work item is not mutable (%ld)", (long) _state);
-}
-
 - (void)setReadyHandler:(void (^)(id context, NSInteger retryCount, MTRAsyncWorkCompletionBlock completion))readyHandler
 {
     [self assertMutable];
@@ -62,7 +59,31 @@ - (void)setCancelHandler:(void (^)(void))cancelHandler
     _cancelHandler = cancelHandler;
 }
 
-- (void)markedEnqueued
+- (void)setBatchingID:(NSUInteger)opaqueBatchingID data:(id)opaqueBatchableData handler:(MTRAsyncWorkBatchingHandler)batchingHandler
+{
+    NSParameterAssert(batchingHandler);
+    [self assertMutable];
+    _batchingID = opaqueBatchingID;
+    _batchableData = opaqueBatchableData;
+    _batchingHandler = batchingHandler;
+}
+
+- (void)setDuplicateTypeID:(NSUInteger)opaqueDuplicateTypeID handler:(MTRAsyncWorkDuplicateCheckHandler)duplicateCheckHandler
+{
+    NSParameterAssert(duplicateCheckHandler);
+    [self assertMutable];
+    _duplicateTypeID = opaqueDuplicateTypeID;
+    _duplicateCheckHandler = duplicateCheckHandler;
+}
+
+- (void)assertMutable
+{
+    NSAssert(_state == MTRAsyncWorkItemMutable, @"work item is not mutable (%ld)", (long) _state);
+}
+
+#pragma mark Management by the work queue (queue lock held)
+
+- (void)markEnqueued
 {
     [self assertMutable];
     _state = MTRAsyncWorkItemEnqueued;
@@ -82,6 +103,7 @@ - (NSInteger)retryCount
 
 - (void)callReadyHandlerWithContext:(id)context completion:(MTRAsyncWorkCompletionBlock)completion
 {
+    //
     NSAssert(_state >= MTRAsyncWorkItemEnqueued, @"work item is not enqueued (%ld)", (long) _state);
     NSInteger retryCount = 0;
     if (_state == MTRAsyncWorkItemEnqueued) {
@@ -105,45 +127,37 @@ - (void)callReadyHandlerWithContext:(id)context completion:(MTRAsyncWorkCompleti
     });
 }
 
-- (BOOL)isComplete
-{
-    return _state == MTRAsyncWorkItemComplete;
-}
-
-- (void)markComplete
-{
-    NSAssert(_state >= MTRAsyncWorkItemEnqueued, @"work item was not enqueued (%ld)", (long) _state);
-    _state = MTRAsyncWorkItemComplete;
-}
-
 - (void)cancel
 {
     if (_state != MTRAsyncWorkItemComplete) {
-        _state = MTRAsyncWorkItemComplete;
         auto cancelHandler = _cancelHandler;
+        [self markComplete];
         if (cancelHandler) {
-            // Note that this does not prevent a race against
-            // the readyHandler calling the work completion.
+            // Note that if the work item was running it may call the work
+            // completion handler before the cancel handler actually runs,
+            // however in this case the work completion handler will return
+            // NO, giving the work code the ability to deal with this race if
+            // necessary.
             dispatch_async(_queue, cancelHandler);
         }
     }
 }
 
-- (void)setBatchingID:(NSUInteger)opaqueBatchingID data:(id)opaqueBatchableData handler:(MTRAsyncWorkBatchingHandler)batchingHandler
+- (BOOL)isComplete
 {
-    NSParameterAssert(batchingHandler);
-    [self assertMutable];
-    _batchingID = opaqueBatchingID;
-    _batchableData = opaqueBatchableData;
-    _batchingHandler = batchingHandler;
+    return _state == MTRAsyncWorkItemComplete;
 }
 
-- (void)setDuplicateTypeID:(NSUInteger)opaqueDuplicateTypeID handler:(MTRAsyncWorkDuplicateCheckHandler)duplicateCheckHandler
+- (void)markComplete
 {
-    NSParameterAssert(duplicateCheckHandler);
-    [self assertMutable];
-    _duplicateTypeID = opaqueDuplicateTypeID;
-    _duplicateCheckHandler = duplicateCheckHandler;
+    NSAssert(_state >= MTRAsyncWorkItemEnqueued, @"work item was not enqueued (%ld)", (long) _state);
+    _state = MTRAsyncWorkItemComplete;
+
+    // Clear all handlers in case any of them captured this object.
+    _readyHandler = nil;
+    _cancelHandler = nil;
+    _batchingHandler = nil;
+    _duplicateCheckHandler = nil;
 }
 
 @end
@@ -178,9 +192,9 @@ - (void)enqueueWorkItem:(MTRAsyncWorkItem *)item
 {
     NSParameterAssert(item);
     NSAssert(_context, @"context has been lost");
-    [item markedEnqueued];
 
     os_unfair_lock_lock(&_lock);
+    [item markEnqueued];
     [_items addObject:item];
     [self _callNextReadyWorkItem];
     os_unfair_lock_unlock(&_lock);

src/darwin/Framework/CHIPTests/MTRAsyncWorkQueueTests.m

@@ -58,7 +58,7 @@ - (void)testRunItem
         XCTAssertNil(weakItem);
     }];
 
-    [self waitForExpectationsWithTimeout:0.5 handler:nil];
+    [self waitForExpectationsWithTimeout:1 handler:nil];
 
     // see that it only ran once
     XCTAssertEqual(counter, 1);