search

groovy / gmaven

Groovy integration for Maven
http://groovy.github.io/gmaven/
Apache License 2.0
51 stars 21 forks source link

NoExitSecurityManager breaks parallel builds #7

Closed ksperling closed 6 years ago

ksperling commented 6 years ago

SystemNoExitGuard and similar code in the GMaven plugin that tries to set a SecurityManager for a specific block of code is inherently not thread safe.

In practice what happens in my build when I run it multi-threaded is that a first groovy script execution installs the NoExitSecurityManager, and an overlapping execution from another module will then see the NESM as the "existing" SecurityManager and restore it after it's done. The end result is that the overall Maven build hangs after it is finished when the plexus Launcher tries to call exit:

        at org.codehaus.gmaven.plugin.util.NoExitSecurityManager.deny(NoExitSecurityManager.java:37)
        at org.codehaus.gmaven.plugin.util.NoExitSecurityManager.checkExit(NoExitSecurityManager.java:59)
        at java.lang.Runtime.exit(Runtime.java:107)
        at java.lang.System.exit(System.java:971)
        at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:358)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.apache.maven.wrapper.BootstrapMainStarter.start(BootstrapMainStarter.java:39)
        at org.apache.maven.wrapper.WrapperExecutor.execute(WrapperExecutor.java:122)
        at org.apache.maven.wrapper.MavenWrapperMain.main(MavenWrapperMain.java:50)
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.apache.maven.wrapper.BootstrapMainStarter.start(BootstrapMainStarter.java:39)
        at org.apache.maven.wrapper.WrapperExecutor.execute(WrapperExecutor.java:122)
        at org.apache.maven.wrapper.MavenWrapperMain.main(MavenWrapperMain.java:50)
Caused by: java.lang.SecurityException: Use of System.exit() is forbidden
        at org.codehaus.gmaven.plugin.util.NoExitSecurityManager.deny(NoExitSecurityManager.java:37)
        at org.codehaus.gmaven.plugin.util.NoExitSecurityManager.checkExit(NoExitSecurityManager.java:59)
        at java.lang.Runtime.exit(Runtime.java:107)
        at java.lang.System.exit(System.java:971)
        at org.codehaus.plexus.classworlds.launcher.Launcher.main(Launcher.java:364)
        ... 7 more

I'm not quite sure what the purpose of the NESM is in the first place... is System.exit likely to be called by some groovy code that a build script author does not have control over?

If someone wants to put a literal System.exit() into a groovy execution inside their pom.xml, they shouldn't be surprised if it causes the JVM to exit, so this doesn't seem like something the gmaven plugin would need to guard against. Saving/restoring the standard in/out/err file handles falls into the same category. Overall it seems way outside the scope of a small utility plugin like this to manipulate the global security manager and IO streams for the entire JVM.

The approaches I can see to resolve this are

  1. Make save/restore/noexit configurable (opt-out or opt-in) and document that it's not threadsafe
  2. Remove the whole save/restore/noexit code entirely
  3. Serialize all groovy execution via a global lock
  4. Use a global ref-count for managing adding / removing the NESM.

Personally I would tend towards (1) or (2). Approaches (3) and (4) could still be combined with (1). I can look at submitting a PR if there is an agreed approach.

jdillon commented 6 years ago

So FTR I don't remember why I put NESM, may be a side-effect of shell or console integration. I'm included though remove it, unless I can remember why it was needed and/or useful.

keeganwitt commented 6 years ago

I thought the reason was for a script used both in GMaven and invoked directly from command line might have exits in them for that latter purpose that you don't want to happen when invoked from former. Maybe a bit of a special case...

ksperling commented 6 years ago

Looks like the current guard code makes sense primarily for the shell and console mojos where System.out seems to be replaced by some kind of ANSI-aware stream and a stream that redirects into the console GUI window respectively. Whether or not forbidding System.exit() in those cases is much of a muchness I guess.

So I'd propose to add a guard config parameter that defaults to false to the execute mojo and will bypass the NESM and stream save/restore code unless guard == true.

ksperling commented 6 years ago

@keeganwitt I guess that's possible... you'd still have to deal with the SecurityException in some way though (e.g. by using a specific sub-class and catching it in the mojo) to simulate a clean-ish exit of the groovy script.

Seems for those sorts of things a more robust approach might be to optionally fork off a new JVM similar to surefire.

jdillon commented 6 years ago

So I'm pretty sure its a side effect of integrating the console (and maybe shell) goals which have code that call System.exit, but I'll double check. For execute I don't see any reason its needed, especially since its being such a PITA. The protection is useful, as its really hard to debug when maven just appears to randomly exit, but given the issues with Java and how the SM is generally handled, it may not be worth the complexity.

update doesn't look like groovy:shell or groovy:console are presently calling System.exit, maybe it was something from early days. I'll prepare a PR that rips this stuff out.

jdillon commented 6 years ago

If you can give https://github.com/groovy/gmaven/pull/8 a whirl and see its behaving better.

NOTE: I don't normally use the MT options of Maven so I tend to not run into these problems but I get that folks like that feature, I just found it was too problematic ;-)

ksperling commented 6 years ago

Sorry for the slow response, yes using a gmaven build off that branch fixes my issue :+1:

(I had to change the litmus-testsupport dependency to version 1.9, otherwise dependency resolution failed for me due to it's dependency junit-ext which doesn't seem to be in any of the relevant repos.)

jdillon commented 6 years ago

cool, will merge and move forward, a lot of deps need to be updated like litmus, but I didn't want to change too much at one time.

jdillon commented 6 years ago

I bumped litmus-testsupport to 1.9, got this hooked up to travis-ci as well. will look at a 2.1 release shortly. Maybe then follow up with a 2.2 with larger changes to modernize some other dependencies.

jdillon commented 6 years ago

http://repo1.maven.org/maven2/org/codehaus/gmaven/groovy-maven-plugin/2.1/