Graceful shutdown of the JVMs executing the unit tests, in case of a wait time out. This way, JVMs are properly closed and operations like flushing test coverage to disk are properly finished. (#25)

Author: tpetrov-lp

pom.xml

@@ -7,7 +7,7 @@
     <groupId>com.clevertap</groupId>
     <artifactId>supertest-maven-plugin</artifactId>
     <packaging>maven-plugin</packaging>
-    <version>1.12</version>
+    <version>1.13</version>
     <description>A wrapper for Maven's Surefire Plugin, with advanced re-run capabilities.
     </description>
     <name>supertest-maven-plugin</name>
@@ -60,6 +60,13 @@
             <version>4.12</version>
             <scope>test</scope>
         </dependency>
+
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>4.11.0</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <distributionManagement>

src/main/java/com/clevertap/maven/plugins/supertest/SuperTestMavenPlugin.java

@@ -1,5 +1,6 @@
 package com.clevertap.maven.plugins.supertest;
 
+import com.clevertap.maven.plugins.supertest.util.ProcessHelper;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
@@ -38,6 +39,8 @@ public class SuperTestMavenPlugin extends AbstractMojo {
     private static final String TEST_REGEX = "-Dtest=(.*?)(\\s|$)";
     private static final Pattern TEST_REGEX_PATTERN = Pattern.compile(TEST_REGEX);
 
+    private final ProcessHelper processHelper = new ProcessHelper();
+
     private ExecutorService pool;
 
     @Parameter(defaultValue = "${project}", readonly = true)
@@ -56,6 +59,9 @@ public class SuperTestMavenPlugin extends AbstractMojo {
     @Parameter(property = "shellNoActivityTimeout", readonly = true, defaultValue = "300")
     Integer shellNoActivityTimeout;
 
+    @Parameter(property = "gracefulShutdownTimeout", readonly = true, defaultValue = "60")
+    Integer gracefulShutdownTimeout;
+
     @Parameter(property = "includes" )
     List<String> includes;
 
@@ -190,11 +196,51 @@ public int runShellCommand(final String command, final String commandDescriptor)
         if (exited) {
             return proc.exitValue();
         } else {
-            proc.destroyForcibly();
+            shutdown(proc);
+
             return 1;
         }
     }
 
+    private void shutdown(Process proc) throws InterruptedException {
+        // tries to gracefully shutdown first
+        gracefullyShutdown(proc);
+
+        if (!proc.waitFor(gracefulShutdownTimeout, TimeUnit.SECONDS)) {
+            // if the process is still alive after the graceful period is over,
+            // kill it forcibly; this may have some undesired effects (e.g. test coverage
+            // info might not be flushed to disk), but we better kill it, than waiting forever
+            getLog().info("Process did not shutdown within " + gracefulShutdownTimeout
+                    + " sec, killing it forcibly now...");
+            proc.destroyForcibly();
+        } else {
+            getLog().info("Process shutdown successfully upon time out.");
+        }
+    }
+
+    private void gracefullyShutdown(Process proc) {
+        if (processHelper.isUnixProcess(proc)) {
+            // Calling proc.destroy() does not seem to properly propagate SIGTERM to
+            // the child processes. As a result killing the maven process this way does not
+            // make the forked JVMs write coverage data to disk. This is the reason why
+            // we find the process leaves and send them a SIGTERM signal.
+            try {
+                List<Long> leaves = processHelper.getLeaves(processHelper.getPid(proc));
+
+                for (long pid : leaves) {
+                    processHelper.sendSIGTERM(pid);
+                }
+            } catch (Exception e) {
+                getLog().error("Error while gracefully shutting down.", e);
+                proc.destroy();
+            }
+        } else {
+            // getLeaves(...) is not supported on non-Unix systems, that's why we just destroy
+            // with all trade-offs coming from that (see above)
+            proc.destroy();
+        }
+    }
+
     private String[] getShellCommandAsArray(String command) {
         // this is what Runtime.getRuntime().exec(...) is doing internally
         StringTokenizer tokenizer = new StringTokenizer(command);

src/main/java/com/clevertap/maven/plugins/supertest/util/OSUtil.java

@@ -0,0 +1,16 @@
+package com.clevertap.maven.plugins.supertest.util;
+
+import java.util.Locale;
+
+public class OSUtil {
+    private static final String OS = System.getProperty("os.name").toLowerCase(Locale.ENGLISH);
+
+    private OSUtil() {
+        // prevents instance creation
+    }
+
+    public static boolean isUnix() {
+        return (OS.contains("nix") || OS.contains("nux") || OS.contains("aix")
+                || OS.contains("mac"));
+    }
+}

src/main/java/com/clevertap/maven/plugins/supertest/util/ProcessHelper.java

@@ -0,0 +1,123 @@
+package com.clevertap.maven.plugins.supertest.util;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class ProcessHelper {
+    private final Runtime runtime;
+
+    public ProcessHelper() {
+        this(Runtime.getRuntime());
+    }
+
+    ProcessHelper(Runtime runtime) {
+        this.runtime = runtime;
+    }
+
+    /**
+     * Gets the process id of a java process.
+     *
+     * @return the pid found or -1 in case of an error on unsupported OS
+     */
+    @SuppressWarnings("java:S3011")
+    public long getPid(Process process) {
+        // java 8 does not have a handy util for getting pid, that's why reflection is needed
+        // (since java 9, process management is much better)
+        long pid = -1;
+
+        try {
+            if (isUnixProcess(process)) {
+                Field field = process.getClass().getDeclaredField("pid");
+                field.setAccessible(true);
+
+                try {
+                    pid = field.getLong(process);
+                } finally {
+                    field.setAccessible(false);
+                }
+            }
+        } catch (Exception e) {
+            pid = -1;
+        }
+
+        return pid;
+    }
+
+    @SuppressWarnings({"java:S1872"})
+    public boolean isUnixProcess(Process process) {
+        return process.getClass().getName().equals("java.lang.UNIXProcess");
+    }
+
+    public Process sendSIGINT(Process process) throws IOException {
+        return sendSIGINT(getPid(process));
+    }
+
+    public Process sendSIGINT(long pid) throws IOException {
+        return runtime.exec("kill -SIGINT " + pid);
+    }
+
+    public Process sendSIGTERM(Process process) throws IOException {
+        return sendSIGTERM(getPid(process));
+    }
+
+    public Process sendSIGTERM(long pid) throws IOException {
+        return runtime.exec("kill " + pid);
+    }
+
+    // depends on the presence of pgrep command
+    public List<Long> getChildren(long pid) {
+        if (!OSUtil.isUnix()) {
+            throw new UnsupportedOperationException("Not supported on non-Unix OS.");
+        }
+
+        String children = runCommand("pgrep -P " + pid);
+        return Arrays.stream(children.split("\n")).filter(c -> !c.isEmpty()).map(Long::parseLong)
+                .collect(Collectors.toList());
+    }
+
+    /**
+     * Gets the process ids of all leaves in the process tree of the provided pid.
+     */
+    public List<Long> getLeaves(long pid) {
+        List<Long> leaves = new ArrayList<>();
+        List<Long> children = getChildren(pid);
+
+        for (long child : children) {
+            List<Long> childLeaves = getLeaves(child);
+
+            if (childLeaves.isEmpty()) {
+                leaves.add(child);
+            } else {
+                leaves.addAll(childLeaves);
+            }
+        }
+
+        return leaves;
+    }
+
+    private String runCommand(String command) {
+        try {
+            Process proc = runtime.exec(command);
+            StringBuilder output = new StringBuilder();
+
+            try (BufferedReader reader =
+                    new BufferedReader(new InputStreamReader(proc.getInputStream()))) {
+                String line;
+
+                while ((line = reader.readLine()) != null) {
+                    output.append(line).append('\n');
+                }
+            }
+
+            return output.toString();
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+    }
+}

src/test/java/com/clevertap/maven/plugins/supertest/util/ProcessHelperTest.java

@@ -0,0 +1,95 @@
+package com.clevertap.maven.plugins.supertest.util;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assume.assumeTrue;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.internal.util.collections.Sets;
+
+public class ProcessHelperTest {
+    private Runtime runtime;
+    private ProcessHelper processHelper;
+
+    @Before
+    public void setUp() {
+        runtime = mock(Runtime.class);
+        processHelper = new ProcessHelper(runtime);
+    }
+
+    @Test
+    public void testGetPid() throws IOException {
+        assumeTrue(OSUtil.isUnix());
+
+        Process process = Runtime.getRuntime().exec("ls");
+        long pid = processHelper.getPid(process);
+        assertTrue(pid > 0);
+    }
+
+    @Test
+    public void testIsUnixProcess() throws IOException {
+        assertTrue(OSUtil.isUnix());
+        assertTrue(processHelper.isUnixProcess(Runtime.getRuntime().exec("ls")));
+    }
+
+    @Test
+    public void testGetChildren() throws IOException {
+        assumeTrue(OSUtil.isUnix());
+
+        doReturn(mockProcess("54321\n65432\n")).when(runtime).exec("pgrep -P 12345");
+        List<Long> children = processHelper.getChildren(12345L);
+        assertEquals(Arrays.asList(54321L, 65432L), children);
+    }
+
+    @Test
+    public void testGetLeaves() throws IOException {
+        assumeTrue(OSUtil.isUnix());
+
+        doReturn(mockProcess("54321\n65432\n")).when(runtime).exec("pgrep -P 12345");
+        doReturn(mockProcess("111\n222\n")).when(runtime).exec("pgrep -P 54321");
+        doReturn(mockProcess("")).when(runtime).exec("pgrep -P 65432");
+        doReturn(mockProcess("")).when(runtime).exec("pgrep -P 111");
+        doReturn(mockProcess("")).when(runtime).exec("pgrep -P 222");
+
+        Set<Long> expectedLeaves = Sets.newSet(222L, 111L, 65432L);
+        Set<Long> actualLeaves = new HashSet<>(processHelper.getLeaves(12345L));
+
+        assertEquals(expectedLeaves, actualLeaves);
+    }
+
+    @Test
+    public void testSendSIGINT() throws Exception {
+        Process killProcess = mock(Process.class);
+        when(runtime.exec("kill -SIGINT 12345")).thenReturn(killProcess);
+
+        Process process = processHelper.sendSIGINT(12345L);
+        assertEquals(killProcess, process);
+    }
+
+    @Test
+    public void testSendSIGTERM() throws Exception {
+        Process killProcess = mock(Process.class);
+        when(runtime.exec("kill 12345")).thenReturn(killProcess);
+
+        Process process = processHelper.sendSIGTERM(12345L);
+        assertEquals(killProcess, process);
+    }
+
+    private Process mockProcess(String output) {
+        Process process = mock(Process.class);
+        when(process.getInputStream())
+                .thenReturn(new ByteArrayInputStream(output.getBytes()));
+
+        return process;
+    }
+}