Skip to content

Bug (Race-Condition) - Refactor use of System.(get|set|clear)Property #1597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gndrm opened this issue Dec 3, 2018 · 2 comments
Closed

Bug (Race-Condition) - Refactor use of System.(get|set|clear)Property #1597

gndrm opened this issue Dec 3, 2018 · 2 comments
Milestone

Comments

@gndrm
Copy link
Contributor

gndrm commented Dec 3, 2018

Description

When using the openapi-generator-maven-plugin in a multi module Build with maven failsafe-plugin for Integration Tests, the generator is not working properly. Race conditions arise, since surefires AbstractSurefireMojo (Base class for IntegrationTestMojo) clones the Properties Object returned by System.getProperties() before the run and sets it as the new Properties in System afterwards.

Now consider the following sequence of statements:

  1. surefire clones System properties
  2. generator-maven-plugin sets Properties
  3. surefire resets System Properties (generators properties are gone)
  4. DefaultGenerator tries to read Properties

This breaks the plugin configuration and causes unintended behaviour.

openapi-generator version

3.3.0, but as far as i can see, the newest version doesn't change anything in regard to this behaviour.

Suggest a fix/enhancement

remove all references to System Properties, since those can be modified from everywhere and everybody inside the same JVM. Instead define a POJO-GeneratorProperties Object, which gets passed from GodeGenMojo to DefaultGenerator. Alternatively (what i have done so far) implement static class which initially clones the System Properties in a InheritableThreadLocal field and provides access to the cloned Object via (get|set|clear)Property Methods. This way you still have easy access to manipulate those properties, but you are not running the risk of losing Properties in a multi-threaded environment. This would also guarantee right behaviour, if you want to run two parallel executions of generator-plugin.

@jmini
Copy link
Member

jmini commented Dec 4, 2018

Thank you for starting this!

Using System.setProperty(..) is a bad practice, we discussed it in #845 (comment)

@jimschubert jimschubert added this to the 4.0.0 milestone Sep 30, 2019
@jimschubert
Copy link
Member

Closing this, as it was included in 4.0.0-beta and the contribution seems to work very well.

Thanks for the cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants