Skip to content

Commit

Permalink
[Darwin] MTRAsyncCallbackWorkQueue tsan fix and API strengthening (#2…
Browse files Browse the repository at this point in the history
  • Loading branch information
jtung-apple authored and pull[bot] committed Jun 23, 2023
1 parent c685b5d commit 7109708
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 13 deletions.
3 changes: 3 additions & 0 deletions src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ typedef void (^MTRAsyncCallbackReadyHandler)(id context, NSUInteger retryCount);
- (void)invalidate;

// Work items may be enqueued from any queue or thread
// Note: Once a work item is enqueued, its handlers cannot be modified
- (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item;

// TODO: Add a "set concurrency width" method to allow for more than 1 work item at a time
Expand All @@ -71,10 +72,12 @@ typedef void (^MTRAsyncCallbackReadyHandler)(id context, NSUInteger retryCount);

// Called by the creater of the work item when async work is done and should
// be removed from the queue. The work queue will run the next work item.
// Note: This must only be called from within the readyHandler
- (void)endWork;

// Called by the creater of the work item when async work should be retried.
// The work queue will call this workItem's readyHandler again.
// Note: This must only be called from within the readyHandler
- (void)retryWork;
@end

Expand Down
75 changes: 65 additions & 10 deletions src/darwin/Framework/CHIP/MTRAsyncCallbackWorkQueue.mm
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,13 @@ - (void)retryWork:(MTRAsyncCallbackQueueWorkItem *)workItem;
@end

@interface MTRAsyncCallbackQueueWorkItem ()
@property (nonatomic, readonly) os_unfair_lock lock;
@property (nonatomic, strong, readonly) dispatch_queue_t queue;
@property (nonatomic, readwrite) NSUInteger retryCount;
@property (nonatomic, strong) MTRAsyncCallbackWorkQueue * workQueue;
@property (nonatomic, readonly) BOOL enqueued;
// Called by the queue
- (void)markedEnqueued;
- (void)callReadyHandlerWithContext:(id)context;
- (void)cancel;
@end
Expand Down Expand Up @@ -72,6 +75,13 @@ - (NSString *)description

- (void)enqueueWorkItem:(MTRAsyncCallbackQueueWorkItem *)item
{
if (item.enqueued) {
MTR_LOG_ERROR("MTRAsyncCallbackWorkQueue enqueueWorkItem: item cannot be enqueued twice");
return;
}

[item markedEnqueued];

os_unfair_lock_lock(&_lock);
item.workQueue = self;
[self.items addObject:item];
Expand Down Expand Up @@ -163,12 +173,14 @@ @implementation MTRAsyncCallbackQueueWorkItem
- (instancetype)initWithQueue:(dispatch_queue_t)queue
{
if (self = [super init]) {
_lock = OS_UNFAIR_LOCK_INIT;
_queue = queue;
}
return self;
}

- (void)invalidate
// assume lock is held
- (void)_invalidate
{
// Make sure we don't leak via handlers that close over us, as ours must.
// This is a bit odd, since these are supposed to be non-nullable
Expand All @@ -181,6 +193,38 @@ - (void)invalidate
_cancelHandler = nil;
}

- (void)invalidate
{
os_unfair_lock_lock(&_lock);
[self _invalidate];
os_unfair_lock_unlock(&_lock);
}

- (void)markedEnqueued
{
os_unfair_lock_lock(&_lock);
_enqueued = YES;
os_unfair_lock_unlock(&_lock);
}

- (void)setReadyHandler:(MTRAsyncCallbackReadyHandler)readyHandler
{
os_unfair_lock_lock(&_lock);
if (!_enqueued) {
_readyHandler = readyHandler;
}
os_unfair_lock_unlock(&_lock);
}

- (void)setCancelHandler:(dispatch_block_t)cancelHandler
{
os_unfair_lock_lock(&_lock);
if (!_enqueued) {
_cancelHandler = cancelHandler;
}
os_unfair_lock_unlock(&_lock);
}

- (void)endWork
{
[self.workQueue endWork:self];
Expand All @@ -196,24 +240,35 @@ - (void)retryWork
- (void)callReadyHandlerWithContext:(id)context
{
dispatch_async(self.queue, ^{
if (self.readyHandler == nil) {
os_unfair_lock_lock(&self->_lock);
MTRAsyncCallbackReadyHandler readyHandler = self->_readyHandler;
NSUInteger retryCount = self->_retryCount;
if (readyHandler) {
self->_retryCount++;
}
os_unfair_lock_unlock(&self->_lock);

if (readyHandler == nil) {
// Nothing to do here.
[self endWork];
} else {
self.readyHandler(context, self.retryCount);
self.retryCount++;
readyHandler(context, retryCount);
}
});
}

// Called by the work queue
- (void)cancel
{
dispatch_async(self.queue, ^{
if (self.cancelHandler != nil) {
self.cancelHandler();
}
[self invalidate];
});
os_unfair_lock_lock(&self->_lock);
dispatch_block_t cancelHandler = self->_cancelHandler;
[self _invalidate];
os_unfair_lock_unlock(&self->_lock);

if (cancelHandler) {
dispatch_async(self.queue, ^{
cancelHandler();
});
}
}
@end
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ NS_ASSUME_NONNULL_BEGIN
@interface MTRAsyncCallbackWorkQueue ()
// The MTRDevice object is only held and passed back as a reference and is opaque to the queue
- (instancetype)initWithContext:(id _Nullable)context queue:(dispatch_queue_t)queue;

// Called by DeviceController at device clean up time
- (void)invalidate;
@end

NS_ASSUME_NONNULL_END

0 comments on commit 7109708

Please sign in to comment.