Skip to content
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

Plugin is not thread safe #14

Closed
gjoranv opened this issue Aug 29, 2018 · 6 comments
Closed

Plugin is not thread safe #14

gjoranv opened this issue Aug 29, 2018 · 6 comments
Assignees
Labels

Comments

@gjoranv
Copy link
Contributor

gjoranv commented Aug 29, 2018

When testing this plugin in multithreaded maven builds, it failed in 3 out of 5 runs.

Maven creates a new instance of the mojo for each thread. So in general, for a plugin to be thread safe, it cannot use any system resources, e.g. writing to common tmp dirs or system properties. Also, it should not use any non-final static fields.

In all cases when the build fails, we see stack traces from two of our maven modules that run ToolFacade.run at exactly the same time (stack trace is identical for both modules):

09:35:05 Caused by: org.apache.maven.plugin.MojoExecutionException: Failed to execute JavaCC
09:35:05     at org.codehaus.mojo.javacc.ToolFacade.run (ToolFacade.java:100)
09:35:05     at org.codehaus.mojo.javacc.JavaCCMojo.processGrammar (JavaCCMojo.java:161)
09:35:05     at org.codehaus.mojo.javacc.AbstractJavaCCMojo.execute (AbstractJavaCCMojo.java:464)
09:35:05     at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)
09:35:05     at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:208)
...

In about half the cases, we don't see any more details. But the remaining builds report a NullPointerException for one of the modules:

09:35:05 Caused by: java.lang.NullPointerException
09:35:05     at com.helger.pgcc.parser.JavaCCParserInternals.setinsertionpoint (JavaCCParserInternals.java:176)
09:35:05     at com.helger.pgcc.parser.JavaCCParser.ClassOrInterfaceDeclaration (JavaCCParser.java:1860)
09:35:05     at com.helger.pgcc.parser.JavaCCParser.TypeDeclaration (JavaCCParser.java:1780)
09:35:05     at com.helger.pgcc.parser.JavaCCParser.CompilationUnit (JavaCCParser.java:1629)
09:35:05     at com.helger.pgcc.parser.JavaCCParser.javacc_input (JavaCCParser.java:136)
09:35:05     at com.helger.pgcc.parser.Main.mainProgram (Main.java:296)
09:35:05     at org.codehaus.mojo.javacc.JavaCC.execute (JavaCC.java:557)
09:35:05     at org.codehaus.mojo.javacc.ToolFacade.run (ToolFacade.java:96)
09:35:05     at org.codehaus.mojo.javacc.JavaCCMojo.processGrammar[INFO] Downloading from local central mirror: http:/ (JavaCCMojo.java:161)
09:35:05    at org.codehaus.mojo.javacc.AbstractJavaCCMojo.execute (AbstractJavaCCMojo.java:464)
09:35:05     at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)

It seems that JavaCCMojo.processGrammar is a common entry point. A quick fix might be to synchronize the method if the root cause is difficult to find.

@phax phax self-assigned this Aug 29, 2018
@phax phax added the bug label Aug 29, 2018
@phax
Copy link
Collaborator

phax commented Aug 29, 2018

Point taken.
Short term: synchronize call to e "pgcc.parser.Main"
Long term: avoid global variables there :)

@gjoranv
Copy link
Contributor Author

gjoranv commented Dec 3, 2018

@phax Any updates on this issue? You are of course right that synchronization must be placed in the static methods of parser.Main and not in the mojo like I suggested.

@phax
Copy link
Collaborator

phax commented Dec 3, 2018

Of course not :( I forgot - sorry.
I will fix this quickly and make you a 4.1.1 release.

@phax
Copy link
Collaborator

phax commented Dec 3, 2018

Version 4.1.1 will be on Maven Central within the next half hour (I hope).

@phax phax closed this as completed Dec 3, 2018
@gjoranv
Copy link
Contributor Author

gjoranv commented Dec 4, 2018

@phax, thanks a lot! We'll test this and provide an update.

@gjoranv
Copy link
Contributor Author

gjoranv commented Dec 10, 2018

Your changes seems to do the trick. I ran 8 builds with no errors in the environment that was most prone to the issue. I sent a PR to mark the plugin as thread safe to get rid of the maven warning, see #16.
Thanks a lot!

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

No branches or pull requests

2 participants