stryker-mutator / stryker-net

Mutation testing for .NET core and .NET framework!
https://stryker-mutator.io
Apache License 2.0
1.75k stars 176 forks source link

fix(Concurrency): Concurrency issue with hashset on MutantPlacer #2912

Closed richardwerkman closed 2 months ago

richardwerkman commented 2 months ago

closes #2899

richardwerkman commented 2 months ago

@dupdob do you see any issues with this change? I don't know the reason behind these fields being static. Probably for performance reasons, but if there was another reason this PR might have some unexpected effects.

sonarcloud[bot] commented 2 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

dupdob commented 2 months ago

@richardwerkman : sorry, I was off last week and missed your message. I did not had an opportunity to look into this yet (will do early this week). My gut answer is that it is probably easier to keep the static bits and protect them with a lock than to go through the likely hassle to make them instance variable. But as said earlier, I need to take a look at the code to reach for conclusion.

IIRC, the use of staticwas chosen to simplify the writing of orchestrators and mutators, i.e. those were helper (and stateless) classes.

Having a look at the code, I double down on my advice of simply adding a lock(...) statement to protect register engine. It will have negligible impact on performance and remove any concurrency related problem (regarding mutant placers).

richardwerkman commented 2 months ago

Thanks for your view on this. I already did the work so we could not take the work needed into account. When I read the code I noticed it isn't stateless completely, that's why I changed it away from static.

@rouke-broersma what do you think? Revert to static and add a lock, or keep the change as is.

dupdob commented 2 months ago

I opened PR #2916 to demonstrate my alternate approach, which has the great benefit of requiring only a few changes.