New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class Mocks Not Thread-Safe #328

Open
alanterranova opened this Issue Jan 16, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@alanterranova

alanterranova commented Jan 16, 2017

Hi Erik,

I’m debugging thread safety in OCMock 3 and have an example test case that demonstrates some faulty behavior. In short, it looks like invoking a class method from a background thread while a class mock is being restored causes crashes due to unrecognized selectors.

Strangely, this isn’t a problem with OCMock 2. I suspect 1ad7292 may have something to do with it.

I realize this is a contrived example, but it’s flaky enough to prevent me from upgrading my project from v2 to v3.

Thanks,

Alan

@kballard

This comment has been minimized.

kballard commented Jan 9, 2018

I just hit this same issue, with OCMock 3.3 (which the release notes claims makes mocking thread-safe).

I believe the root issue here is OCMock's class mocking isa-swizzles the metaclass prior to installing methods. This means that background threads may start interacting with the metaclass while it's in the process of installing methods, which is pretty bad.

@kballard

This comment has been minimized.

kballard commented Jan 9, 2018

Since I'm not sure if moving the metaclass swizzle to the end is safe (I don't know if any of the intervening code relies on it already being installed), I believe a safe fix is to reorder two lines in -setupForwarderForClassMethodSelector::

diff --git a/Pods/OCMock/Source/OCMock/OCClassMockObject.m b/Pods/OCMock/Source/OCMock/OCClassMockObject.m
index f8a8180..39b5f22 100644
--- a/Pods/OCMock/Source/OCMock/OCClassMockObject.m
+++ b/Pods/OCMock/Source/OCMock/OCClassMockObject.m
@@ -148,8 +148,8 @@ - (void)setupForwarderForClassMethodSelector:(SEL)selector
 
     Class metaClass = object_getClass(mockedClass);
     IMP forwarderIMP = [originalMetaClass instanceMethodForwarderForSelector:selector];
-    class_replaceMethod(metaClass, selector, forwarderIMP, types);
     class_addMethod(metaClass, aliasSelector, originalIMP, types);
+    class_replaceMethod(metaClass, selector, forwarderIMP, types);
 }
@kballard

This comment has been minimized.

kballard commented Jan 9, 2018

@erikdoe I assume you're getting notifications for issue updates anyway, but just in case, I'm mentioning you as this is a nasty issue that has been causing spurious test failures for us for a long time and I'm surprised that this ticket never got a response.

@kballard

This comment has been minimized.

kballard commented Oct 18, 2018

@erikdoe Can you please take a look at this? It's a real issue with a trivial fix.

FWIW, it turns out that uninstalling class mocks isn't thread safe either, which doesn't seem to be a solvable short of simply never destroying the dynamically-created class pairs, but in my experience the uninstall issue crops up a lot less frequently than the install issue did.

erikdoe added a commit that referenced this issue Oct 22, 2018

@erikdoe

This comment has been minimized.

Owner

erikdoe commented Oct 22, 2018

Sorry. I should have responded earlier. My concern is that the failing test provided in the issue tests a scenario that, as far as I can tell, cannot be supported by OCMock. It has always been clear that OCMock is only thread-safe to a certain degree. Repeatedly calling a method while setting up and removing a mock would, as far as I am aware, require most of the operations on the class to be atomic, and I don't see a way how to achieve this.

The fix provided by @kballard looks benign, but it doesn't fix the issue reported. In fact, there is no test that I can think of that would fail without the change but pass with it. That said, the change is small and it feels like it brings the two operations into a "more correct" sequence. So I have accepted it now.

@erikdoe erikdoe added the enhancement label Oct 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment