Execute Mock response handlers un-synchronized

[leonard84]

Prior to this commit the response handler of a mock was called in a
synchronized method. This could lead to deadlocks when one mock was
blocking in its response handler to wait on another mock.

fixes #583, #1882

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2c7db77) 80.44% compared to head (8c9cb9a) 80.76%.
Report is 12 commits behind head on master.

Files	Patch %	Lines
...g/spockframework/mock/runtime/MockInteraction.java	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1885      +/-   ##
============================================
+ Coverage     80.44%   80.76%   +0.32%     
- Complexity     4337     4374      +37     
============================================
  Files           441      442       +1     
  Lines         13534    13744     +210     
  Branches       1707     1728      +21     
============================================
+ Hits          10888    11101     +213     
+ Misses         2008     2005       -3     
  Partials        638      638              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Vampire]

I think we need to immediately rollback this PR and release 2.4-M3. This majorly breaks interaction matching in a multi-threaded situation and does not remove the blocking that was intended to be removed at all, due to the locking that was introduced.

The test that was added is not meaningful, it will always succeed after waiting for 30 seconds. Before the MockInteraction was made thread-safe with locks, it finished fast but was also not meaningful as it would never fail. Using a lock-free thread-safe collection instead of locks or moving the respond call to outside the write lock would make the test succeed faster again but still never fail. Just fixing the test is not an option though, as the logic is majorly broken as race conditions are introduced due to the now missing synchronization that was removed.

If you for example have two interactions with 1 * for the get in the test, then it can easily happen, that the first thread uses the synchronized findInteraction which uses scope.match which uses interaction.isExhausted which uses acceptedInvocations.size().

Then before the first thread calls accept which would record the interaction invocation, the second thread does the same and gets the same interaction.

Now both threads call accept on the same interaction, where the first succeeds and the second throws a TooManyInvocationsError due to maxCount being exhausted, which is then swallowed in said test by the executor service and makes the test fail once it is rewritten to better test the situation.

Besides that, the accept in the anonymous class in InteractionScope.addInteraction probably has the same problems as MockInteraction had before it was made thread-safe due to reading and modifying state that is now no longer protected by the synchronization that was removed.

It probably should just be made illegal to block in one response handler waiting for another response handler.

Otherwise, someone has to come up with a clever change that fixes this while not breaking current logic like these changes. But I guess for this some major refactoring would be necessary, so that only the response handler call is somehow moved to after the synchronization. Maybe by returning the response handler from accept and then invoking it after the synchronization or something like that, if that works.

Also, this change, or a an according other fix would need to be listed as potentially breaking change, as before the response handlers were synchronized and thus were safe to modify state, while with the response handlers not being synchronized anymore this is no longer the case and users would have to care about thread-safety themselves.