More thread safety fixes needed

[myronmarston]

Ran across this gist that demonstrates some thread safety issues with 3.0.0.beta2 (and 2.14, of course):

https://gist.github.com/ordinaryzelig/f3a48b2456eac28bbbca

Thought it would be useful for when we dig more into this.

[cupakromer]

I've forked it for now just to make sure it's kept around: https://gist.github.com/cupakromer/a40d975e9c958733cf08

[xaviershay]

neat, this is really handy

[xaviershay]

so we can't support constant stubbing and thread-safe mode at the same time.

that's kind of awkward, since I think constant stubbing is often the best way to write code (dependency inversion isn't always clearer).

I think I'd be ok with making a call that rspec-mocks does not support parallel threaded execution. The only time you should be using it is in unit specs, which should be hella fast anyways, right?

[myronmarston]

so we can't support constant stubbing and thread-safe mode at the same time.

Hmmm...I can see it being problematic, but it doesn't seem much different from stubbing a method on a globally accessible object, right? Two specs that both want to stub the same constant in different ways can't run at the same time, obviously, but that seems like a specific instance of a more general problem of code being coupled to global state.

I can see that we may want to do something to disallow constant stubbing in specs that run in parallel, though. I also like the idea discussed in rspec/rspec-core#1254 of making it possible to opt-in to parallelism in certain parts of your suite but not others.

[xaviershay]

for posterity @samphippen

[3:21pm] samphippen: xshay: I have an idea
[3:21pm] samphippen: it's quite bad...
[3:21pm] samphippen: it involves some kind of constant rewriting based on the local thread trying to access them using tracepoint
[3:21pm] xshay: lol
[3:21pm] xshay: wow
[3:21pm] xshay: that is terrible
[3:22pm] samphippen: I suspect you could make it work

[myronmarston]

Honestly, you could probably just make the constant stubbing lazy (using const_missing) and have it check some thread state when doing that. Not sure I want to go down that path, though.

[hakanai]

I think I'd be ok with making a call that rspec-mocks does not support parallel threaded execution.
The only time you should be using it is in unit specs, which should be hella fast anyways, right?

What if you're testing a component which internally uses threads?

context "when a callback is provided"
  it "calls the callback once per item"
    callback = double(:callback)
    expect(callback).to receive(:found).exactly(24).times
    component.search('query', callback)
  end
end

With rspec-mocks 3.4.1, this fails for me claiming that the callback has not been called 24 times, even though I can see from the debugger that it is called exactly 24 times. My guess would be that the maps it's using to keep track of this are not synchronised. If you modify the code to print to the console for every time it's called, the issue mysteriously stops happening because console output forces a sync. Jamming a sleep in after the call also fixes the test. Both of these are of course cheating.

[JonRowe]

Given what you've described, it's more likely the expectation is being verified before your method call is "complete".

[hakanai]

What actually happens is that the tasks which are spun off in the background all complete on their different threads before the method call returns, but the threads themselves are in a pool, so the threads themselves don't complete. But I think from the point of view of concurrency, these calls to the callback don't "happen-before" the check rspec does at the end of the test, because it isn't synchronising whatever structures it's storing this information in, so effectively it isn't being verified "after" the calls have been made.

I plan to try and make a standalone test for it. I'm just not sure yet whether such a test would reproduce the issue if written entirely in Ruby, or whether I have to call the callback from Java code on JRuby to get this behaviour.

[JonRowe]

The moment component.search('query', callback) returns to RSpec, the expectation will be verified, the number of calls that RSpec has received by then will be used. You can fix this by adding a check to check for when your thread pool is drained.

[hakanai]

All the tasks submitted to the thread pool complete before the method call returns. This I already know, because the same unit test written in Java passes. (But jMock has an option to synchronise its mocks and if I don't use that, the test fails in a fashion rather similar to what happens with rspec-mocks.)

[JonRowe]

That doesn't apply here, RSpec can't see beyond it's own thread AFAIK