ConfigurationLoader isn't thread safe due to its use of ConfigSlurper #423

Closed
schillingr opened this Issue Feb 17, 2016 · 5 comments

Projects

None yet

2 participants

@schillingr

Running Specifications using 1 thread per spec results in seemingly random failures when loading GebConfig.groovy. With 5 threads I rarely saw the issue, but over 5 I was able to reproduce roughly 50% of the time.

In debugging the issue it seems that ConfigSlurper itself is not thread-safe when used on a Class. I made a quick project that demonstrates the problem. You may need to run the test multiple times to have it fail, but eventually it will.

https://github.com/schillingr/groovy-configslurper-not-thread-safe

I solved this locally by placing a synchronized block around the ConfigurationLoader call in GebSpec which I'm running tests on now.

@erdi erdi added a commit to geb/geb that closed this issue Feb 20, 2016
@erdi erdi Ensure that the config classes are not slurped concurrently as Config…
…Slurper.parse(Script, URL) modifies meta class of the passed script class and is hence not thread safe when passed instances of the same class.

Fixes geb/issues#423
4c1f295
@erdi
Member
erdi commented Feb 20, 2016

Yes, ConfigSlurper.parse(Script, URL) was clearly not designed to be called concurrently with Script instaces of the same class. I put synchronization on the config class around calling that method.

@erdi erdi added the Bug label Feb 20, 2016
@erdi erdi added this to the 0.13.1 milestone Feb 20, 2016
@erdi erdi self-assigned this Feb 20, 2016
@erdi
Member
erdi commented Feb 20, 2016

Out of curiosity, why did you go for parallelizing your suite using threads instead of using multiuple test JVMs (like when setting maxParallelForks to be greater than 1 on Gradle test tasks) which is less prone to issues like these?

@schillingr

Mostly due to resource constraints. Forking seemed to use considerably more
RAM which made threading more efficient. We run on the order of 8k tests
with every build so the efficiency is becoming more important for us.

On Sat, Feb 20, 2016, 3:53 PM Marcin Erdmann notifications@github.com
wrote:

Out of curiosity, why did you go for parallelizing your suite using
threads instead of using multiuple test JVMs (like when setting
maxParallelForks to be greater than 1 on Gradle test tasks) which is less
prone to issues like these?


Reply to this email directly or view it on GitHub
#423 (comment).

@erdi
Member
erdi commented Feb 21, 2016

Thanks for the explanation. Hopefully the changes I made based on your reports will enable you to get better efficiency.

Btw, you can work around this issue by changing your config script to be a resource instead of a class - it will be parsed from text every time essentially creating a new script class with every call.

@schillingr

I appreciate all your help with this, I'll look into making it a resource.

On Sun, Feb 21, 2016, 6:40 AM Marcin Erdmann notifications@github.com
wrote:

Thanks for the explanation. Hopefully the changes I made based on your
reports will enable you to get better efficiency.

Btw, you can work around this issue by changing your config script to be a
resource instead of a class - it will be parsed from text every time
essentially creating a new script class with every call.


Reply to this email directly or view it on GitHub
#423 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment