OCMockObject is not thread safe #235
Conversation
| @@ -90,8 +98,7 @@ - (id)initWithRecorder:(OCMRecorder *)aRecorder | |||
| - (void)dealloc | |||
| { | |||
| [recorder release]; | |||
| if(globalState == self) | |||
| globalState = nil; | |||
| NSAssert([NSThread currentThread].threadDictionary[OCMGlobalStateKey] != self, @"Unexpected dealloc while set as the global state"); | |||
| [super dealloc]; | |||
There was a problem hiding this comment.
Why introduce an assert here? And shouldn't the object for OCMGlobalStateKey be removed from the thread dictionaory at this point?
There was a problem hiding this comment.
Yes the object for OCMGlobalStateKey should always be removed from the thread dictionary at this point, the assert was a "just in case" thing. I don't mind removing it.
ian-twilightcoder
changed the title
|
In case you are wondering, this PR is good and could be merged. However, the performance issues with iOS 9 are now confirmed (see #253), and the only workaround I can think of would impact threading. So, I think I'll postpone merging this one until we have a bit more clarity about the runtime issue. |
|
Thanks for the update, would love to see this in a release soon so I can stop building my own version of OCMock. ;-) |
|
It just occurred to me that there are other places in OCMock that aren't thread-safe. Consider this code from When two threads hit this at the same time, it's possible that one thread sets Just wondering whether we really want to make OCMock completely thread safe. Or could we describe a "level" of thread-safety we're aiming for? |
|
I would say that it's reasonable to say that you're only allowed to call -stopMocking once, kind of like -[NSTimer invalidate] and similar methods, in which case I don't think you need to worry about the threading. Although -restoreMetaClass is called from more than just -stopMocking, so yeah maybe mockedClass and originalMetaClass need similar guards. I admit that I didn't really do an exhaustive check over all the classes in OCMock, just the ones that were involved when I hit crashes mocking user defaults in a multithreaded app. I suppose we could say that OCMock is thread safe enough such that the mocks can be used on multiple threads, but their creation and destruction is generally intended to be done on a single thread in +setUp, +tearDown, -setUp, -tearDown, and the various -testXXX methods? Or I can try and find more time to check all the other classes if you prefer? |
|
Is this by any chance related to the following error? We've had some periodic failures during unit tests on OSX. I thought that unit tests are run on the main thread, so I'm not sure what the issue is. |
|
Unit test methods are always invoked on the main thread, but they can spawn threads like anything else. (XCTestExpectation supports this). That said, the implication here is that -[OCMInvocationMatcher matchesInvocation:] is somehow modifying the the OCMockObject's invocations array on the same thread. (Since the access is locked, it's impossible for another thread to be doing the modification.) That should be an existing bug as far as I can tell. Also this hasn't even been merged in to master yet, I'm still waiting for input from Erik on that. |
|
@iangithubusername, thanks for the answer. Just for clarification, I was curious if your PR would address these types of issues. It sounds like this is a separate issue. |
|
It depends, I'd have to see the full crash log. If you have another thread in some OCMock methods, then this change has a good chance of addressing your issue. In my case I was seeing crashes like this too, mostly from a partial mock I'd made on +[NSUserDefaults standardUserDefaults] which gets accessed from pretty much every thread in my project. After this change all my crashes went away, however if you're seeing this in strictly single threaded code then this change shouldn't make a difference. |
|
+1 A lot of objects that we mock are themselves thread-safe, which other code takes advantage of, but then the thread-safety guarantees of those objects fall apart when being mocked. Really looking forward to this PR being merged. |
|
@erikdoe What do you think about this one? Do you think it's good enough to say that tear down has to happen from only one thread, and only after everything is done using the mock? Other than that caveat I think this change does make the mock objects thread safe. |
|
@iangithubusername I noticed in testing that OCObserverMockObject does not have any |
|
Good point, updated the PR. Although there's another problem with a retain cycle, I'm awaiting feedback from Erik as to how he wants that handled. |
|
@carllindberg I tried just nil'ing out the invocation targets but those are used by a couple different things like partial mocks to find their original object. What I ended up doing instead was hanging onto a copy of the invocation with self removed and arguments retained. That seems to work out alright and lets me remove the blacklist for reference counting methods too. |
|
I forgot that -retainArguments makes copies of c-strings as well -- that is the reason for avoiding retainArguments on methods which have char pointers, since the pointer address will change, breaking pointer comparisons needed by -anyPointer etc. (and it would crash if it tried dereferencing those pointers). I think we may be best off writing our own NSInvocation category method "retainObjectArguments". That can go through the arguments (avoiding the target and return value), and for each one which is an object, add it to an NSArray. (If the argument is a block, make sure to call -copy first.) Then, add that array as an associated object to the NSInvocation. This approach should avoid the char * and target problem, but still make sure that object arguments are retained. |
|
I considered doing that too, just grabbing all the objects out of the invocation and adding them to a counted set. I thought it would be good to keep each invocation distinct and having them line up with the original invocation, just in case something ever removes things out of the invocation list then it would be easy to also remove from the retained invocations. Since the retained invocation copy isn't used for anything but to retain the arguments, the char * thing doesn't really matter. I suppose I could've dropped all of the non-object things out of the invocation too. I don't know, there didn't really seem to be an obvious way to solve the problem. This one seems to work pretty well but if anyone really wants something else that's fine too, I just want to get this change integrated. |
|
If you just add the set or array as an associated object on the invocation instance, then the arguments are retained and they will be released naturally whenever the invocation gets dealloced. In other words, it's just a better version of -retainArguments for our purposes (which just adds objects to an internal NSArray). It would also fix the other place which currently needs to avoid -retainArguments. Your version should work most of the time, though it will fail matching if "self" happens to be an argument other than the target (since it won't be copied over). |
|
My version isn't affecting the matching at all. The original invocation is untouched, and the new sidecar invocation is used solely for retaining the target, arguments, and return value while not retaining self to avoid the retain cycle. Personally I'm not a fan of associated objects, they're too non-obvious to me and I only really use them in a last resort situation. What I could do is change invocations to be a map table whose keys are the invocations and whose values are a retained collection of just the object target/arguments/return value that aren't self. |
|
Ah, OK. Associated objects aren't bad at all; they are best used by category methods which give a better API but they are far far better than other mechanisms (like an external map table, the older approach). So, what you have is basically a more complicated version using an NSInvocation to retain the arguments instead of an NSArray, and additional storage on the copied invocations, but the effect would be the same. Adding it as an associated object would be pretty clean and requires no cleanup code anywhere else, and that method would also be appropriate to use in the other place which avoids retaining arguments, but whatever Erik prefers really. |
|
They're not bad, I just find them to be kind of non-obvious. It's a little clearer to me to use a map table since there's an obvious place to keep it (OCMockObject). Also "-retainObjectArgumentsExcludingObject:" seems like a kind of funky API to add onto NSInvocation. But let me look at the other place, maybe it's not as clean there. |
|
I think they are reasonably obvious when self-contained in a category method. It'd be funky API to add to a general-purpose category, of course, but for an OCMock-specific category it is what is needed ;-) Oh... might be a good idea to copy block objects instead of just retaining them, in case they are a stack block. |
|
Oh yeah, that's why I was manually setting up a copy of the invocation in the first place; I didn't want to mess around figuring out what all needed to be copied or retained. In the interest of second guessing NSInvocation behavior as little as possible and letting -retainArguments just do its thing, I think I prefer the way it is right now and leaving OCMInvocationMatcher alone. We'll see what Erik says though. |
|
OK, that is interesting. You could also just store the copied invocation as an associated object on the original invocation then, and it will be cleaned up at the appropriate time without need to store them separately. Hopefully we would not run into a situation where a stack block was left on the original NSInvocation. If you make the other place which is avoiding char * retainArguments use your method, that could also fix problems with method which have both char * and object arguments. Determining a block isn't too hard though... |
|
Merged up to master and updated. |
|
@iangithubusername @erikdoe so, I've spent some time investigating the performance issues with OCMock, which this branch somehow exacerbates. The tests run fine when run from Xcode, but they slow down significantly when run using xctool (which many of us use, including travis-ci by default). It turns out that the issue is that xctool's otest-shim is injected to the test bundle by xctool, when you use it to run the tests. For its own purposes, it defines |
|
Just a quick heads up. I'm not ignoring this thread and I do want this functionality to end up in OCMock. Unfortunately, though, I simply haven't found the time to look into something as complex as this. I'm expecting to spend time on OCMock over the holiday season. |
|
Thanks for checking in, did you get a chance to have a look at this? |
|
So, I finally had some time to look into this and, unfortunately, this is not a straight merge at the moment. In detail:
Notice frame 2. An |
|
Make OCMockObject thread safe. Synchronize on the four mutable array instance variables during access to ensure their consistency. Retain everything retrieved from the arrays if the retrieved objects ever get used outside of the synchronized chunks. Retain captured invocation arguments since the arguments could otherwise be deallocated at any time. The invocation can't outright retain its arguments since that would cause a retain cycle, so use a customized method to retain arguments that filters out self. Add all the reference counting methods (-retainCount, -retain, -release, -autorelease) to OCPartialMockObject's selector black list. They're a recipe for stack overflow since partial mocks set up a forwarder for -retain. The forwarder would capture the call to -retain as an NSInvocation, OCMockObject now retains the invocation arguments, which calls -retain on the original object again, which hits the forwarder method again, and so on and so forth. Make OCMMacroState thread safe as well. Having a global macro state doesn't work. With locking it doesn't crash, but if two threads are recording at the same time then one of the macro states gets stepped on. Change the global state to be a per-thread state. Avoid a use-after-dealloc race by not trying to clear the global state in -dealloc and making the macros exception safe such that the +endXXXMacro calls always happen even if the captured invocation throws an exception. Make OCObserverMockObject thread safe. Synchronize on the two mutable array instance variables during access to ensure their consistency. #171
| // is a block though (the argument type at index 0 is always "@" even if | ||
| // the target is a Class or block), and in practice it's OK since you | ||
| // can't mock a block. | ||
| [retainedArguments addObject:target]; |
There was a problem hiding this comment.
I gave an implementation to check whether an object is a block in the comments to this pull request... it's probably safer to test the object directly rather than rely on the types, I'd guess.
Might be able to make a helper function / macro to reduce some of the repetitive code, but looks good.
There was a problem hiding this comment.
I thought I'd wrote this last week... But if I were to guess I'd say that the different block types are more likely to share an encoding type than they are to share a common non-NSObject superclass. Though I'm sure either way it's relying on implementation detail. I'll see what @erikdoe says, hopefully we can close this out soon once and for all.
There was a problem hiding this comment.
I think if you have code compiled with non-ARC blocks can be very different encoded types. Even with ARC I have seen some differences, I think. Maybe they have become more consistent on newer compilers but I have seen inconsistencies in the past (and OCMock could be running on lots of setups). I think dispatch_block_t may be encoded very differently, and it's also easily possible to pass a block into a method argument which is typed to accept any object. The common non-NSObject block superclass, while an implementation detail, is highly unlikely to change, and is a runtime check of the actual instance -- it should be much safer.
There was a problem hiding this comment.
I tried in ARC and non-ARC, all block types encode as @? even dispatch_block_t. So as far as I can tell the code as written is solid.
I actually pulled the identifying code from OCMIsObjectType, I just added another check for a case I saw where block arguments were included as part of the type. So I think this is stylistically the right thing to do, it positively identifies the argument type even if the actual passed argument is nil, and it seems to work in all cases. Of course I'll defer to @erikdoe but I think what's in the pull request is good.
There was a problem hiding this comment.
What about older versions of Xcode? OCMock gets used in lots of development environments.
And also, the type check will fail if an argument is typed as "id", but a block happens to be passed into it (say addObject: on an NSArray, if code creates an array of blocks, say).
But yes, it's up to Erik ;-)
|
Hi @erikdoe what's the next step for this one? |
|
So, I finally, finally merged this. Many apologies. I've had a few crazy months... The changes I made on top of the PR are:
|
|
Thanks for merging this. However without the OCMMacroState thread dictionary changes and try/finally cleanup, anything that uses the OCMock 3 macros to create mocks that get used in multiple threads will very quickly crash. |
|
Not sure I understand. As far as I can tell the use of the thread dictionary makes the actual use of the macro thread-safe, it ensures that when two threads in parallel use the stub/expect/verify macros, the macros still work. If we use these macros only from one thread, then there should be no problem, not? |
|
I don't remember the exact details, just that things accessing +[OCMMacroState globalState] were getting the wrong state object which was causing messages to go to the wrong mock. |
|
From -[OCMockObject forwardingTargetForSelector:] as I recall. I think there was a background thread in that method, and the main thread was in the middle of setting up a different mock object. |
|
Hmm. Let's assume we do this: These two calls should result in two invocations of |
|
Okay, thinking about it again, do you have the following scenario in mind? If If that's the case then the other Could you test whether the version as is does work in your scenarios? I'm asking because the code for OCMock is quite complex already and I only want to add stuff that's absolutely necessary to implement the features. I'm not asking for a unit test. Just a quick heads up whether current master would work for you. |
Make OCMockObject thread safe.
Synchronize on the four mutable array instance variables during access to ensure their consistency.
Retain everything retrieved from the arrays if the retrieved objects ever get used outside of the synchronized chunks.
Retain captured invocation arguments since the arguments could otherwise be deallocated at any time. The invocation can't outright retain its arguments since that would cause a retain cycle, so use a customized method to retain arguments that filters out self.
Add all the reference counting methods (-retainCount, -retain, -release, -autorelease) to OCPartialMockObject's selector black list. They're a recipe for stack overflow since partial mocks set up a forwarder for -retain. The forwarder would capture the call to -retain as an NSInvocation, OCMockObject now retains the invocation arguments, which calls -retain on the original object again, which hits the forwarder method again, and so on and so forth.
Make OCMMacroState thread safe as well.
Having a global macro state doesn't work. With locking it doesn't crash, but if two threads are recording at the same time then one of the macro states gets stepped on. Change the global state to be a per-thread state.
Avoid a use-after-dealloc race by not trying to clear the global state in -dealloc and making the macros exception safe such that the +endXXXMacro calls always happen even if the captured invocation throws an exception.
Make OCObserverMockObject thread safe.
Synchronize on the two mutable array instance variables during access to ensure their consistency.
Tested by building all schemes in the Xcode project, running the static analyzer on all of them, and running the unit tests on all of them.
Used a built copy of OCMock in my OS X project and verified that all of my project's unit tests completed successfully against the built copy of OCMock.
#171