Skip to content

Commit 832ca63

Browse files
committed
Make expectation recording thread-safe.
To handle a mocked invocation, we do the following: 1. Get the first stub that matches the invocation. 2. If the stub is an expectation, remove the stub from the arrays of stubs and expectations. 3. Pass the invocation to the stub. The first two steps are currently not atomic, so there is a race condition. For example: 1. Add expectation 1 for method A. 2. Add expectation 2 for same method A. 3. [Thread 1] Call method A. 4. [Thread 2] Call method A. 5. [Thread 1] Get expectation 1 since it matches the invocation. 6. [Thread 2] Get expectation 1 since it matches the invocation. 7. [Thread 1] Remove expectation 1 from the arrays. 8. [Thread 2] Remove expectation 1 from the arrays. In the above example, the invocations of the same method in Thread 1 and Thread 2 both match expectation 1; expectation 2 does not get matched. The solution is to make the matching and the removal atomic. We can do so by wrapping the relevant code in `@synchronized(stubs)`. (Even though `stubForInvocation:` also performs `@synchronized(stubs)`, we can feel free to do the same in the caller without fear of deadlocks since `@synchronized` uses a recursive lock.) Added a new unit test that was failing before the fix and is now passing after the fix.
1 parent 37c9b03 commit 832ca63

File tree

2 files changed

+42
-30
lines changed

2 files changed

+42
-30
lines changed

Source/OCMock/OCMockObject.m

Lines changed: 25 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -408,44 +408,39 @@ - (BOOL)handleInvocation:(NSInvocation *)anInvocation
408408
[self assertInvocationsArrayIsPresent];
409409
[self addInvocation:anInvocation];
410410

411-
OCMInvocationStub *stub = [self stubForInvocation:anInvocation];
412-
if(stub == nil)
413-
return NO;
411+
OCMInvocationStub *stub = nil;
412+
@synchronized(stubs) {
413+
stub = [self stubForInvocation:anInvocation];
414+
if(stub == nil)
415+
return NO;
414416

415-
// Retain the stub in case it ends up being removed because we still need it at the end for handleInvocation:
416-
[stub retain];
417+
// Retain the stub in case it ends up being removed because we still need it at the end for handleInvocation:
418+
[stub retain];
417419

418-
BOOL removeStub = NO;
419-
@synchronized(expectations)
420-
{
421-
if([expectations containsObject:stub])
420+
@synchronized(expectations)
422421
{
423-
OCMInvocationExpectation *expectation = [self _nextExpectedInvocation];
424-
if(expectationOrderMatters && (expectation != stub))
422+
if([expectations containsObject:stub])
425423
{
426-
[NSException raise:NSInternalInconsistencyException
427-
format:@"%@: unexpected method invoked: %@\n\texpected:\t%@",
428-
[self description], [stub description], [[expectations objectAtIndex:0] description]];
429-
}
424+
OCMInvocationExpectation *expectation = [self _nextExpectedInvocation];
425+
if(expectationOrderMatters && (expectation != stub))
426+
{
427+
[NSException raise:NSInternalInconsistencyException
428+
format:@"%@: unexpected method invoked: %@\n\texpected:\t%@",
429+
[self description], [stub description], [[expectations objectAtIndex:0] description]];
430+
}
430431

431-
// We can't check isSatisfied yet, since the stub won't be satisfied until we call
432-
// handleInvocation: since we'll still have the current expectation in the expectations array, which
433-
// will cause an exception if expectationOrderMatters is YES and we're not ready for any future
434-
// expected methods to be called yet
435-
if(![(OCMInvocationExpectation *)stub isMatchAndReject])
436-
{
437-
[expectations removeObject:stub];
438-
removeStub = YES;
432+
// We can't check isSatisfied yet, since the stub won't be satisfied until we call
433+
// handleInvocation: since we'll still have the current expectation in the expectations array, which
434+
// will cause an exception if expectationOrderMatters is YES and we're not ready for any future
435+
// expected methods to be called yet
436+
if(![(OCMInvocationExpectation *)stub isMatchAndReject])
437+
{
438+
[expectations removeObject:stub];
439+
[stubs removeObject:stub];
440+
}
439441
}
440442
}
441443
}
442-
if(removeStub)
443-
{
444-
@synchronized(stubs)
445-
{
446-
[stubs removeObject:stub];
447-
}
448-
}
449444

450445
@try
451446
{

Source/OCMockTests/OCMockObjectTests.m

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,4 +1156,21 @@ - (void)testTryingToCreateAnInstanceOfOCMockObjectRaisesAnException
11561156
XCTAssertThrows([[OCMockObject alloc] init]);
11571157
}
11581158

1159+
#pragma mark thread safety
1160+
1161+
- (void)testExpectationsAreThreadSafe
1162+
{
1163+
size_t taskCount = 10000;
1164+
1165+
for (size_t i = 0; i < taskCount; i++) {
1166+
[[mock expect] lowercaseString];
1167+
}
1168+
1169+
dispatch_apply(taskCount, dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^(size_t iteration) {
1170+
[mock lowercaseString];
1171+
});
1172+
1173+
[mock verify];
1174+
}
1175+
11591176
@end

0 commit comments

Comments
 (0)