Skip to content

Commit 9225790

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 9225790

File tree

2 files changed

+45
-30
lines changed

2 files changed

+45
-30
lines changed

Source/OCMock/OCMockObject.m

+26-30
Original file line numberDiff line numberDiff line change
@@ -408,44 +408,40 @@ - (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+
{
414+
stub = [self stubForInvocation:anInvocation];
415+
if(stub == nil)
416+
return NO;
414417

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

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

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

450446
@try
451447
{

Source/OCMockTests/OCMockObjectTests.m

+19
Original file line numberDiff line numberDiff line change
@@ -1156,4 +1156,23 @@ - (void)testTryingToCreateAnInstanceOfOCMockObjectRaisesAnException
11561156
XCTAssertThrows([[OCMockObject alloc] init]);
11571157
}
11581158

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

0 commit comments

Comments
 (0)