Skip to content

Fix for OCMockObject thread-safety#175

Closed
kettch wants to merge 2 commits intoerikdoe:masterfrom
kettch:OCMock-thread-safe
Closed

Fix for OCMockObject thread-safety#175
kettch wants to merge 2 commits intoerikdoe:masterfrom
kettch:OCMock-thread-safe

Conversation

@kettch
Copy link
Copy Markdown

@kettch kettch commented Jan 27, 2015

Synchronizes uses of stubs/expectations/invocations to avoid
thread-safety issues.

Synchronizes uses of stubs/expectations/invocations to avoid
thread-safety issues.
@erikdoe
Copy link
Copy Markdown
Owner

erikdoe commented Feb 17, 2015

Thanks for looking into this. I have a couple of comments regarding this pull request.

While putting the @synchronised in as is may work it would be good to apply them with more precision. For example, it would seem that the @synchronised is not needed in _nextExptectedInvocation because this method is only called from handleInvocation which is already wrapped in an @Synchronised. I'm also unsure about wrapping the entire body of handleInvocation: into one large block. Maybe it would be better to extract access to the arrays in question and only guard that access.

Then there is the issue of OCMMacroState. This class is clearly not thread-safe due to the use of a global variable. I would shy away from calling OCMock thread-safe without also addressing this issue.

And with something as complex as this, I really think we would need at least some test coverage.

@kettch
Copy link
Copy Markdown
Author

kettch commented Feb 17, 2015

Indeed, I can look into adding a bit more precision to this. This was just based so far on a quick fix I had done in my specific use cases, so I could verify this was working.

I must admit that I'm not that familiar with the internals of OCMock, so the title of the issue was merely a shortcut to what I was seeing in my integration of it throughout my tests.

I'll certainly try fine-tuning the @synchronize that I added, and adding a few tests to check the behaviour. However, I'm not sure I'd be very confident digging into OCMMacroState (I hadn't even heard of that one before today...). That could be left for another pull request if someone feels up to it and maybe knows that part better than me?

@erikdoe erikdoe mentioned this pull request May 11, 2015
@erikdoe erikdoe changed the title Fix for OCMockObject thread-safety (#171) Fix for OCMockObject thread-safety May 11, 2015
@erikdoe
Copy link
Copy Markdown
Owner

erikdoe commented Oct 3, 2015

Closing this as #235 looks more promising.

@erikdoe erikdoe closed this Oct 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants