Skip to content

Commit 8e87870

Browse files
pre-commit-ci[bot]woody-apple
authored andcommitted
[pre-commit.ci] auto fixes from pre-commit.com hooks
for more information, see https://pre-commit.ci
1 parent ae7c320 commit 8e87870

2 files changed

Lines changed: 283 additions & 26 deletions

File tree

src/darwin/Framework/CHIP/MTRCommissioningOperation.mm

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,12 @@ @implementation MTRCommissioningOperation {
8484
os_unfair_lock _stateLock;
8585
// Test-only one-shot fault injection. When YES, the next call to
8686
// _armPostPASEWatchdog will simulate a dispatch_source_create failure
87-
// (return NO without arming the timer) and clear the flag. Read and
88-
// written only on _delegateQueue, mirroring _postPASEWatchdog itself.
87+
// (return NO without arming the timer) and clear the flag. Reads and
88+
// writes are serialized by _stateLock so that the test setter can run
89+
// synchronously from any queue and still publish the flag write before
90+
// returning -- without it, tests had to rely on FIFO ordering on
91+
// _delegateQueue and explicit dispatch_sync flushes to observe the
92+
// injected fault.
8993
BOOL _forceNextArmFailureForTesting;
9094
}
9195

@@ -203,9 +207,17 @@ - (BOOL)_armPostPASEWatchdog
203207
// Test-only one-shot fault injection: simulate a
204208
// dispatch_source_create failure so callers can exercise the
205209
// "watchdog could not be armed -> bail out" path without having to
206-
// actually exhaust dispatch sources.
207-
if (_forceNextArmFailureForTesting) {
210+
// actually exhaust dispatch sources. Read-and-clear under _stateLock
211+
// so a setter on any queue is synchronously visible here without
212+
// requiring tests to flush _delegateQueue first.
213+
BOOL forceFailure;
214+
os_unfair_lock_lock(&_stateLock);
215+
forceFailure = _forceNextArmFailureForTesting;
216+
if (forceFailure) {
208217
_forceNextArmFailureForTesting = NO;
218+
}
219+
os_unfair_lock_unlock(&_stateLock);
220+
if (forceFailure) {
209221
MTR_LOG_ERROR("%@ post-PASE watchdog arm forced to fail (testing)", self);
210222
return NO;
211223
}
@@ -584,6 +596,24 @@ - (void)controller:(MTRDeviceController *)controller
584596
return;
585597
}
586598

599+
// Defensive: by the time we land here on a success path the controller
600+
// should already have flipped isWaitingAfterPASEEstablished back to NO
601+
// (from commissionNodeWithID:) which fires the watchdog teardown via
602+
// the setter side effect. But this method is the terminal success
603+
// notification for the operation, so don't depend on that upstream
604+
// invariant -- if a future code path ever reaches success without
605+
// having cleared the flag, an armed watchdog would otherwise survive
606+
// to fire after we have nilled out _delegate and finished commissioning,
607+
// taking the late-fire guard's "replaced" branch but still doing the
608+
// dispatch_source bookkeeping uselessly. Clear the flag synchronously
609+
// and bounce the dispatch_source cancel itself onto _delegateQueue.
610+
os_unfair_lock_lock(&_stateLock);
611+
_isWaitingAfterPASEEstablished = NO;
612+
os_unfair_lock_unlock(&_stateLock);
613+
dispatch_async(_delegateQueue, ^{
614+
[self _cancelPostPASEWatchdog];
615+
});
616+
587617
id<MTRCommissioningDelegate> strongDelegate = _delegate;
588618
MTRDeviceController_Concrete * strongController = _controller;
589619

@@ -799,12 +829,17 @@ + (void)setPostPASEWatchdogIntervalForTesting:(NSTimeInterval)interval
799829

800830
- (void)setForceNextArmFailureForTesting:(BOOL)force
801831
{
802-
// _forceNextArmFailureForTesting is read/written only on
803-
// _delegateQueue (matching _postPASEWatchdog). Bounce onto that
804-
// queue so the test does not have to know which queue it is on.
805-
dispatch_async(_delegateQueue, ^{
806-
self->_forceNextArmFailureForTesting = force;
807-
});
832+
// _forceNextArmFailureForTesting is protected by _stateLock so we can
833+
// publish the write synchronously from any queue (including the test's
834+
// own queue, or _delegateQueue itself). The previous implementation
835+
// bounced onto _delegateQueue via dispatch_async, which required every
836+
// test that wanted to observe the injected fault to manually flush
837+
// _delegateQueue (dispatch_sync(queue, ^{})) before exercising
838+
// _armPostPASEWatchdog -- a footgun a future test would inevitably
839+
// forget.
840+
os_unfair_lock_lock(&_stateLock);
841+
_forceNextArmFailureForTesting = force;
842+
os_unfair_lock_unlock(&_stateLock);
808843
}
809844

810845
@end

0 commit comments

Comments
 (0)