You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by ti...@apache.org on 2019/10/14 01:18:36 UTC

[maven-surefire] 02/02: [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

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 <ti...@apache.org>
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 );
         }
-
     }
 }