This is an automated email from the ASF dual-hosted git repository. tibordigana pushed a commit to branch SUREFIRE-1689 in repository https://gitbox.apache.org/repos/asf/maven-surefire.git
commit 70c8e1b8ad31ba4e19c147bfa3bb49b34262e7b2 Author: tibordigana <[email protected]> AuthorDate: Mon Oct 14 01:04:24 2019 +0200 [SUREFIRE-1689] The fast PpidChecker is switched to the slow 30 seconds PING after the subprocess (/bin/ps -o etime= -p ...) failed with exit 1 --- .../maven/surefire/booter/CommandReader.java | 22 ++--- .../apache/maven/surefire/booter/ForkedBooter.java | 34 ++++--- .../apache/maven/surefire/booter/PpidChecker.java | 100 ++++++++++++++------- .../apache/maven/surefire/booter/ProcessInfo.java | 8 +- .../maven/surefire/booter/PpidCheckerTest.java | 85 +++++++++++++++--- ...Surefire946KillMainProcessInReusableForkIT.java | 2 +- .../surefire/selfdestruct/SelfDestructMojo.java | 11 +-- 7 files changed, 173 insertions(+), 89 deletions(-) diff --git a/surefire-api/src/main/java/org/apache/maven/surefire/booter/CommandReader.java b/surefire-api/src/main/java/org/apache/maven/surefire/booter/CommandReader.java index 15b1dd0..b71aec0 100644 --- a/surefire-api/src/main/java/org/apache/maven/surefire/booter/CommandReader.java +++ b/surefire-api/src/main/java/org/apache/maven/surefire/booter/CommandReader.java @@ -398,22 +398,22 @@ public final class CommandReader if ( inserted ) { CommandReader.this.wakeupIterator(); - insertToListeners( command ); + callListeners( command ); } break; case TEST_SET_FINISHED: CommandReader.this.makeQueueFull(); isTestSetFinished = true; CommandReader.this.wakeupIterator(); - insertToListeners( command ); + callListeners( command ); break; case SHUTDOWN: CommandReader.this.makeQueueFull(); CommandReader.this.wakeupIterator(); - insertToListeners( command ); + callListeners( command ); break; default: - insertToListeners( command ); + callListeners( command ); break; } } @@ -455,7 +455,7 @@ public final class CommandReader } } - private void insertToListeners( Command cmd ) + private void callListeners( Command cmd ) { MasterProcessCommand expectedCommandType = cmd.getCommandType(); for ( BiProperty<MasterProcessCommand, CommandListener> listenerWrapper : CommandReader.this.listeners ) @@ -476,18 +476,8 @@ public final class CommandReader { CommandReader.this.makeQueueFull(); CommandReader.this.wakeupIterator(); - insertToListeners( toShutdown( shutdown ) ); - if ( shutdown.isExit() ) - { - System.exit( 1 ); - } - else if ( shutdown.isKill() ) - { - Runtime.getRuntime().halt( 1 ); - } - // else is default: other than Shutdown.DEFAULT should not happen; otherwise you missed enum case + callListeners( toShutdown( shutdown ) ); } } } - } diff --git a/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java b/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java index 2144e5a..194dc88 100644 --- a/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java +++ b/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ForkedBooter.java @@ -19,6 +19,7 @@ package org.apache.maven.surefire.booter; * under the License. */ +import org.apache.maven.plugin.surefire.log.api.ConsoleLogger; import org.apache.maven.surefire.providerapi.ProviderParameters; import org.apache.maven.surefire.providerapi.SurefireProvider; import org.apache.maven.surefire.report.LegacyPojoStackTraceWriter; @@ -78,6 +79,7 @@ public final class ForkedBooter private ScheduledThreadPoolExecutor jvmTerminator; private ProviderConfiguration providerConfiguration; + private ForkingReporterFactory forkingReporterFactory; private StartupConfiguration startupConfiguration; private Object testSet; @@ -87,7 +89,6 @@ public final class ForkedBooter { BooterDeserializer booterDeserializer = new BooterDeserializer( createSurefirePropertiesIfFileExists( tmpDir, surefirePropsFileName ) ); - pingScheduler = isDebugging() ? null : listenToShutdownCommands( booterDeserializer.getPluginPid() ); setSystemProperties( new File( tmpDir, effectiveSystemPropertiesFileName ) ); providerConfiguration = booterDeserializer.deserialize(); @@ -101,6 +102,12 @@ public final class ForkedBooter } startupConfiguration = booterDeserializer.getProviderConfiguration(); + + forkingReporterFactory = createForkingReporterFactory(); + + ConsoleLogger logger = (ConsoleLogger) forkingReporterFactory.createReporter(); + pingScheduler = isDebugging() ? null : listenToShutdownCommands( booterDeserializer.getPluginPid(), logger ); + systemExitTimeoutInSeconds = providerConfiguration.systemExitTimeout( DEFAULT_SYSTEM_EXIT_TIMEOUT_IN_SECONDS ); AbstractPathConfiguration classpathConfiguration = startupConfiguration.getClasspathConfiguration(); @@ -181,14 +188,18 @@ public final class ForkedBooter } } - private PingScheduler listenToShutdownCommands( Long ppid ) + private PingScheduler listenToShutdownCommands( Long ppid, ConsoleLogger logger ) { - commandReader.addShutdownListener( createExitHandler() ); + PpidChecker ppidChecker = ppid == null ? null : new PpidChecker( ppid ); + commandReader.addShutdownListener( createExitHandler( ppidChecker ) ); AtomicBoolean pingDone = new AtomicBoolean( true ); commandReader.addNoopListener( createPingHandler( pingDone ) ); + PingScheduler pingMechanisms = new PingScheduler( createPingScheduler(), ppidChecker ); + if ( ppidChecker != null ) + { + logger.debug( ppidChecker.toString() ); + } - PingScheduler pingMechanisms = new PingScheduler( createPingScheduler(), - ppid == null ? null : new PpidChecker( ppid ) ); if ( pingMechanisms.pluginProcessChecker != null ) { Runnable checkerJob = processCheckerJob( pingMechanisms ); @@ -244,7 +255,7 @@ public final class ForkedBooter }; } - private CommandListener createExitHandler() + private CommandListener createExitHandler( final PpidChecker ppidChecker ) { return new CommandListener() { @@ -254,6 +265,7 @@ public final class ForkedBooter Shutdown shutdown = command.toShutdownData(); if ( shutdown.isKill() ) { + ppidChecker.stop(); DumpErrorSingleton.getSingleton() .dumpText( "Killing self fork JVM. Received SHUTDOWN command from Maven shutdown hook." + NL @@ -264,6 +276,7 @@ public final class ForkedBooter } else if ( shutdown.isExit() ) { + ppidChecker.stop(); cancelPingScheduler(); DumpErrorSingleton.getSingleton() .dumpText( "Exiting self fork JVM. Received SHUTDOWN command from Maven shutdown hook." @@ -352,8 +365,7 @@ public final class ForkedBooter private void runSuitesInProcess() throws TestSetFailedException, InvocationTargetException { - ForkingReporterFactory factory = createForkingReporterFactory(); - invokeProviderInSameClassLoader( factory ); + createProviderInCurrentClassloader( forkingReporterFactory ).invoke( testSet ); } private ForkingReporterFactory createForkingReporterFactory() @@ -398,12 +410,6 @@ public final class ForkedBooter ); } - private void invokeProviderInSameClassLoader( ForkingReporterFactory factory ) - throws TestSetFailedException, InvocationTargetException - { - createProviderInCurrentClassloader( factory ).invoke( testSet ); - } - private SurefireProvider createProviderInCurrentClassloader( ForkingReporterFactory reporterManagerFactory ) { BaseProviderFactory bpf = new BaseProviderFactory( reporterManagerFactory, true ); diff --git a/surefire-booter/src/main/java/org/apache/maven/surefire/booter/PpidChecker.java b/surefire-booter/src/main/java/org/apache/maven/surefire/booter/PpidChecker.java index d69c419..b525acc 100644 --- a/surefire-booter/src/main/java/org/apache/maven/surefire/booter/PpidChecker.java +++ b/surefire-booter/src/main/java/org/apache/maven/surefire/booter/PpidChecker.java @@ -19,6 +19,7 @@ package org.apache.maven.surefire.booter; * under the License. */ +import javax.annotation.Nonnull; import java.io.File; import java.io.IOException; import java.nio.charset.Charset; @@ -82,11 +83,14 @@ final class PpidChecker PpidChecker( long ppid ) { this.ppid = ppid; - //todo WARN logger (after new logger is finished) that (IS_OS_UNIX && canExecuteUnixPs()) is false } boolean canUse() { + if ( isStopped() ) + { + return false; + } final ProcessInfo ppi = parentProcessInfo; return ppi == null ? IS_OS_WINDOWS || IS_OS_UNIX && canExecuteUnixPs() : ppi.canUse(); } @@ -99,7 +103,6 @@ final class PpidChecker * or this object has been {@link #destroyActiveCommands() destroyed} * @throws NullPointerException if extracted e-time is null */ - @SuppressWarnings( "unchecked" ) boolean isProcessAlive() { if ( !canUse() ) @@ -108,34 +111,26 @@ final class PpidChecker } final ProcessInfo previousInfo = parentProcessInfo; - try + if ( IS_OS_WINDOWS ) { - if ( IS_OS_WINDOWS ) - { - parentProcessInfo = windows(); - checkProcessInfo(); - - // let's compare creation time, should be same unless killed or PID is reused by OS into another process - return previousInfo == null || parentProcessInfo.isTimeEqualTo( previousInfo ); - } - else if ( IS_OS_UNIX ) - { - parentProcessInfo = unix(); - checkProcessInfo(); - - // let's compare elapsed time, should be greater or equal if parent process is the same and still alive - return previousInfo == null || !parentProcessInfo.isTimeBefore( previousInfo ); - } + parentProcessInfo = windows(); + checkProcessInfo(); - throw new IllegalStateException( "unknown platform or you did not call canUse() before isProcessAlive()" ); + // let's compare creation time, should be same unless killed or PID is reused by OS into another process + return !parentProcessInfo.isInvalid() + && ( previousInfo == null || parentProcessInfo.isTimeEqualTo( previousInfo ) ); } - finally + else if ( IS_OS_UNIX ) { - if ( parentProcessInfo == null ) - { - parentProcessInfo = INVALID_PROCESS_INFO; - } + parentProcessInfo = unix(); + checkProcessInfo(); + + // let's compare elapsed time, should be greater or equal if parent process is the same and still alive + return !parentProcessInfo.isInvalid() + && ( previousInfo == null || !parentProcessInfo.isTimeBefore( previousInfo ) ); } + parentProcessInfo = ERR_PROCESS_INFO; + throw new IllegalStateException( "unknown platform or you did not call canUse() before isProcessAlive()" ); } private void checkProcessInfo() @@ -145,11 +140,6 @@ final class PpidChecker throw new IllegalStateException( "error [STOPPED] to read process " + ppid ); } - if ( parentProcessInfo.isError() ) - { - throw new IllegalStateException( "error to read process " + ppid ); - } - if ( !parentProcessInfo.canUse() ) { throw new IllegalStateException( "Cannot use PPID " + ppid + " process information. " @@ -169,6 +159,7 @@ final class PpidChecker ProcessInfoConsumer reader = new ProcessInfoConsumer( Charset.defaultCharset().name() ) { @Override + @Nonnull ProcessInfo consumeLine( String line, ProcessInfo previousOutputLine ) { if ( previousOutputLine.isInvalid() ) @@ -197,6 +188,7 @@ final class PpidChecker private boolean hasHeader; @Override + @Nonnull ProcessInfo consumeLine( String line, ProcessInfo previousProcessInfo ) throws Exception { if ( previousProcessInfo.isInvalid() && !line.isEmpty() ) @@ -256,12 +248,26 @@ final class PpidChecker private static boolean canExecuteLocalUnixPs() { - return new File( "/usr/bin/ps" ).canExecute(); + try + { + return new File( "/usr/bin/ps" ).canExecute(); + } + catch ( SecurityException e ) + { + return false; + } } private static boolean canExecuteStandardUnixPs() { - return new File( "/bin/ps" ).canExecute(); + try + { + return new File( "/bin/ps" ).canExecute(); + } + catch ( SecurityException e ) + { + return false; + } } private static boolean hasWmicStandardSystemPath() @@ -318,6 +324,11 @@ final class PpidChecker return formatter; } + public void stop() + { + stopped = true; + } + /** * Reads standard output from {@link Process}. * <br> @@ -334,7 +345,7 @@ final class PpidChecker this.charset = charset; } - abstract ProcessInfo consumeLine( String line, ProcessInfo previousProcessInfo ) throws Exception; + abstract @Nonnull ProcessInfo consumeLine( String line, ProcessInfo previousProcessInfo ) throws Exception; ProcessInfo execute( String... command ) { @@ -358,7 +369,7 @@ final class PpidChecker } checkValid( scanner ); int exitCode = process.waitFor(); - return exitCode == 0 ? processInfo : INVALID_PROCESS_INFO; + return isStopped() ? ERR_PROCESS_INFO : exitCode == 0 ? processInfo : INVALID_PROCESS_INFO; } catch ( Exception e ) { @@ -381,4 +392,25 @@ final class PpidChecker } } + @Override + public String toString() + { + String args = "ppid=" + ppid + + ", stopped=" + stopped; + + ProcessInfo processInfo = parentProcessInfo; + if ( processInfo != null ) + { + args += ", invalid=" + processInfo.isInvalid() + + ", error=" + processInfo.isError(); + } + + if ( IS_OS_UNIX ) + { + args += ", canExecuteLocalUnixPs=" + canExecuteLocalUnixPs() + + ", canExecuteStandardUnixPs=" + canExecuteStandardUnixPs(); + } + + return "PpidChecker{" + args + '}'; + } } diff --git a/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ProcessInfo.java b/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ProcessInfo.java index a73c33f..fe754f1 100644 --- a/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ProcessInfo.java +++ b/surefire-booter/src/main/java/org/apache/maven/surefire/booter/ProcessInfo.java @@ -19,6 +19,8 @@ package org.apache.maven.surefire.booter; * under the License. */ +import javax.annotation.Nonnull; + /** * Immutable object which encapsulates PID and elapsed time (Unix) or start time (Windows). * <br> @@ -40,12 +42,12 @@ final class ProcessInfo * <br> * <pre>/bin/ps -o etime= -p 123</pre> */ - static ProcessInfo unixProcessInfo( long pid, long etime ) + static @Nonnull ProcessInfo unixProcessInfo( long pid, long etime ) { return new ProcessInfo( pid, etime ); } - static ProcessInfo windowsProcessInfo( long pid, long startTimestamp ) + static @Nonnull ProcessInfo windowsProcessInfo( long pid, long startTimestamp ) { return new ProcessInfo( pid, startTimestamp ); } @@ -61,7 +63,7 @@ final class ProcessInfo boolean canUse() { - return !isInvalid() && !isError(); + return !isError(); } boolean isInvalid() diff --git a/surefire-booter/src/test/java/org/apache/maven/surefire/booter/PpidCheckerTest.java b/surefire-booter/src/test/java/org/apache/maven/surefire/booter/PpidCheckerTest.java index fac18e2..5939b5e 100644 --- a/surefire-booter/src/test/java/org/apache/maven/surefire/booter/PpidCheckerTest.java +++ b/surefire-booter/src/test/java/org/apache/maven/surefire/booter/PpidCheckerTest.java @@ -29,6 +29,8 @@ import java.util.regex.Matcher; import static org.apache.commons.lang3.SystemUtils.IS_OS_UNIX; import static org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS; +import static org.apache.maven.surefire.booter.ProcessInfo.unixProcessInfo; +import static org.apache.maven.surefire.booter.ProcessInfo.windowsProcessInfo; import static org.fest.assertions.Assertions.assertThat; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; @@ -37,6 +39,7 @@ import static org.junit.Assert.fail; import static org.junit.Assume.assumeThat; import static org.junit.Assume.assumeTrue; import static org.powermock.reflect.Whitebox.invokeMethod; +import static org.powermock.reflect.Whitebox.setInternalState; /** * Testing {@link PpidChecker} on a platform. @@ -50,14 +53,21 @@ public class PpidCheckerTest public final ExpectedException exceptions = ExpectedException.none(); @Test - public void shouldHavePpidAsWindows() + public void canExecuteUnixPs() { - assumeTrue( IS_OS_WINDOWS ); + assumeTrue( IS_OS_UNIX ); + assertThat( PpidChecker.canExecuteUnixPs() ) + .as( "Surefire should be tested on real box OS, e.g. Ubuntu or FreeBSD." ) + .isTrue(); + } + @Test + public void shouldHavePidAtBegin() + { long expectedPid = Long.parseLong( ManagementFactory.getRuntimeMXBean().getName().split( "@" )[0].trim() ); PpidChecker checker = new PpidChecker( expectedPid ); - ProcessInfo processInfo = checker.windows(); + ProcessInfo processInfo = IS_OS_UNIX ? checker.unix() : checker.windows(); assertThat( processInfo ) .isNotNull(); @@ -76,18 +86,16 @@ public class PpidCheckerTest } @Test - public void shouldHavePpidAsUnix() + public void shouldHavePid() throws Exception { - assumeTrue( IS_OS_UNIX ); - - assertThat( PpidChecker.canExecuteUnixPs() ) - .as( "Surefire should be tested on real box OS, e.g. Ubuntu or FreeBSD." ) - .isTrue(); - long expectedPid = Long.parseLong( ManagementFactory.getRuntimeMXBean().getName().split( "@" )[0].trim() ); PpidChecker checker = new PpidChecker( expectedPid ); - ProcessInfo processInfo = checker.unix(); + setInternalState( checker, "parentProcessInfo", + IS_OS_UNIX + ? unixProcessInfo( expectedPid, System.currentTimeMillis() - 1_000L ) + : windowsProcessInfo( expectedPid, windowsProcessStartTime( checker ) ) ); + ProcessInfo processInfo = IS_OS_UNIX ? checker.unix() : checker.windows(); assertThat( processInfo ) .isNotNull(); @@ -103,20 +111,50 @@ public class PpidCheckerTest assertThat( processInfo.getTime() ) .isNotNull(); + + assertThat( checker.toString() ) + .contains( "ppid=" + expectedPid ) + .contains( "stopped=false" ) + .contains( "invalid=false" ) + .contains( "error=false" ); + + checker.destroyActiveCommands(); + assertThat( checker.canUse() ) + .isFalse(); + assertThat( (boolean) invokeMethod( checker, "isStopped" ) ) + .isTrue(); + } + + @Test + public void shouldBeStopped() + { + PpidChecker checker = new PpidChecker( 0L ); + checker.stop(); + + assertThat( checker.canUse() ) + .isFalse(); + + exceptions.expect( IllegalStateException.class ); + exceptions.expectMessage( "irrelevant to call isProcessAlive()" ); + + checker.isProcessAlive(); + + fail( "this test should throw exception" ); } @Test public void shouldNotFindSuchPID() { - long ppid = 1000000L; + long ppid = 1_000_000L; PpidChecker checker = new PpidChecker( ppid ); + setInternalState( checker, "parentProcessInfo", ProcessInfo.ERR_PROCESS_INFO ); assertThat( checker.canUse() ) - .isTrue(); + .isFalse(); exceptions.expect( IllegalStateException.class ); - exceptions.expectMessage( "Cannot use PPID " + ppid + " process information. Going to use NOOP events." ); + exceptions.expectMessage( "irrelevant to call isProcessAlive()" ); checker.isProcessAlive(); @@ -124,6 +162,20 @@ public class PpidCheckerTest } @Test + public void shouldNotBeAlive() + { + long ppid = 1_000_000L; + + PpidChecker checker = new PpidChecker( ppid ); + + assertThat( checker.canUse() ) + .isTrue(); + + assertThat( checker.isProcessAlive() ) + .isFalse(); + } + + @Test public void shouldParseEtime() { Matcher m = PpidChecker.UNIX_CMD_OUT_PATTERN.matcher( "38" ); @@ -182,4 +234,9 @@ public class PpidCheckerTest assertThat( (Boolean) invokeMethod( PpidChecker.class, "hasWmicStandardSystemPath" ) ).isTrue(); assertThat( new File( System.getenv( "SystemRoot" ), "System32\\Wbem\\wmic.exe" ) ).isFile(); } + + private static long windowsProcessStartTime( PpidChecker checker ) + { + return (long) checker.windows().getTime(); + } } diff --git a/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire946KillMainProcessInReusableForkIT.java b/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire946KillMainProcessInReusableForkIT.java index e1d09ba..7fab6d0 100644 --- a/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire946KillMainProcessInReusableForkIT.java +++ b/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire946KillMainProcessInReusableForkIT.java @@ -98,7 +98,7 @@ public class Surefire946KillMainProcessInReusableForkIT "-" + shutdownMavenMethod + "-" + shutdownSurefireMethod ) .sysProp( "distinct.classifier", classifierOfDummyDependency ) .sysProp( "surefire.shutdown", shutdownSurefireMethod ) - .sysProp( "selfdestruct.timeoutInMillis", "5000" ) + .sysProp( "selfdestruct.timeoutInMillis", "10000" ) .sysProp( "selfdestruct.method", shutdownMavenMethod ) .sysProp( "testSleepTime", String.valueOf( TEST_SLEEP_TIME ) ) .addGoal( "org.apache.maven.plugins.surefire:maven-selfdestruct-plugin:selfdestruct" ) diff --git a/surefire-its/src/test/resources/surefire-946-self-destruct-plugin/src/main/java/org/apache/maven/plugins/surefire/selfdestruct/SelfDestructMojo.java b/surefire-its/src/test/resources/surefire-946-self-destruct-plugin/src/main/java/org/apache/maven/plugins/surefire/selfdestruct/SelfDestructMojo.java index 61016de..f546206 100644 --- a/surefire-its/src/test/resources/surefire-946-self-destruct-plugin/src/main/java/org/apache/maven/plugins/surefire/selfdestruct/SelfDestructMojo.java +++ b/surefire-its/src/test/resources/surefire-946-self-destruct-plugin/src/main/java/org/apache/maven/plugins/surefire/selfdestruct/SelfDestructMojo.java @@ -64,7 +64,7 @@ public class SelfDestructMojo * * @parameter */ - private long timeoutInMillis = 0; + private long timeoutInMillis; /** * Method of self-destruction: 'exit' will use System.exit (default), 'halt' will use Runtime.halt, 'interrupt' will @@ -77,7 +77,6 @@ public class SelfDestructMojo public void execute() throws MojoExecutionException { - DestructMethod destructMethod = DestructMethod.valueOf( method ); if ( timeoutInMillis > 0 ) @@ -94,7 +93,7 @@ public class SelfDestructMojo private void selfDestruct( DestructMethod destructMethod ) { - getLog().warn( "Self-Destructing NOW." ); + getLog().warn( "[" + destructMethod + "] Self-Destructing NOW." ); switch ( destructMethod ) { case exit: @@ -143,10 +142,9 @@ public class SelfDestructMojo private class SelfDestructionTask extends TimerTask { + private final DestructMethod destructMethod; - private DestructMethod destructMethod; - - public SelfDestructionTask( DestructMethod destructMethod ) + SelfDestructionTask( DestructMethod destructMethod ) { this.destructMethod = destructMethod; } @@ -156,6 +154,5 @@ public class SelfDestructMojo { selfDestruct( destructMethod ); } - } }
