paulbutcher / ScalaMock

Native Scala mocking framework
http://scalamock.org/
MIT License
502 stars 99 forks source link

fix: make all CallLog operations synchronized #462

Closed tkroman closed 6 months ago

tkroman commented 1 year ago

Pull Request Checklist

Purpose

We are getting some ConcurrentModificationExceptions here. The exact use-case is not particularly well-designed but essentially what happens is that if you call a method on a mock in a different thread than the one that does something like eventually { (mock.foo _).verify(bar) }, you will get a CME. In case you are not familiar with eventually, it's basically a while (block-fails) repeat-block

Both [edge|use]-case and this fix are admittedly weird and there's a number of better ways to solve this but given that original code admits just usingsynchronized, I thought this would be OK.

barkhorn commented 1 year ago

Hi and thanks for the PR. At the moment it is missing unit tests for the changed code. Especially as this says fix in the title, I would expect the original problem to be reproduced in a unit test. Given the implications of making something core to the library synchronized, this could break other people's code in the process, and from what you wrote about the CME it's not clear why this is a bug or if this is due to you using the wrong mix-in. You should be using AsyncMockFactory for async test cases.

To progress this, please document what the broken scenario is with an example, and ensure all changes are unit tested.

tkroman commented 1 year ago

Hi! Thanks for the feedback and for the advice. I will still try to do a unit-test/reproduction because I feel that regardless of alternatives this implementation has synchronized modification already, which, coupled with non-synchronized iteration, is still not completely correct. I'll get back to you when I have a reproduction.

barkhorn commented 6 months ago

The codebase has changed significantly in the main branch, so i'm closing this. Feel free to rebase and resubmit if you want to