Merge pull request #1271 from hcoles/feature/cleanup_process_code

Double check thread status before reporting dead minion

pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java

@@ -9,20 +9,21 @@
 import org.pitest.coverage.CoverageResult;
 import org.pitest.process.ProcessArgs;
 import org.pitest.process.WrappingProcess;
+import org.pitest.util.CommunicationThread;
 import org.pitest.util.ExitCode;
 
 public class CoverageProcess {
 
   private final WrappingProcess             process;
-  private final CoverageCommunicationThread crt;
+  private final CommunicationThread crt;
 
   public CoverageProcess(final ProcessArgs processArgs,
       final CoverageOptions arguments, final ServerSocket socket,
       final List<String> testClasses, final Consumer<CoverageResult> handler) {
     this.process = new WrappingProcess(socket.getLocalPort(), processArgs,
         CoverageMinion.class);
-    this.crt = new CoverageCommunicationThread(socket, arguments, testClasses,
-        handler);
+
+    this.crt = new CommunicationThread(socket, new SendData(arguments, testClasses), new Receive(handler));
   }
 
   public void start() throws IOException, InterruptedException {
@@ -36,6 +37,12 @@ public ExitCode waitToDie() {
       while (!maybeExit.isPresent() && this.process.isAlive()) {
         maybeExit = this.crt.waitToFinish(10);
       }
+
+      // Either the monitored process died, or the thread ended.
+      // Check the thread one last time to try and avoid reporting
+      // an error code if it was the process that went down first
+      maybeExit = this.crt.waitToFinish(10);
+
       return maybeExit.orElse(ExitCode.MINION_DIED);
     } finally {
       this.process.destroy();

pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java

@@ -2,27 +2,32 @@
 
 import java.io.IOException;
 import java.net.ServerSocket;
-import java.util.HashMap;
+import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.pitest.mutationtest.MutationStatusMap;
 import org.pitest.mutationtest.MutationStatusTestPair;
 import org.pitest.mutationtest.engine.MutationDetails;
+import org.pitest.mutationtest.engine.MutationIdentifier;
 import org.pitest.process.ProcessArgs;
 import org.pitest.process.WrappingProcess;
+import org.pitest.util.CommunicationThread;
 import org.pitest.util.ExitCode;
 
 public class MutationTestProcess {
 
-  private final WrappingProcess                 process;
-  private final MutationTestCommunicationThread thread;
+  private final WrappingProcess process;
+  private final CommunicationThread thread;
+  private final Map<MutationIdentifier, MutationStatusTestPair> idMap;
 
   public MutationTestProcess(final ServerSocket socket,
       final ProcessArgs processArgs, final MinionArguments arguments) {
     this.process = new WrappingProcess(socket.getLocalPort(), processArgs,
         MutationTestMinion.class);
-    this.thread = new MutationTestCommunicationThread(socket, arguments,
-            new HashMap<>());
+
+    this.idMap = new ConcurrentHashMap<>();
+    this.thread = new CommunicationThread(socket, new SendData(arguments), new Receive(idMap));
 
   }
 
@@ -34,7 +39,7 @@ public void start() throws IOException, InterruptedException {
   public void results(final MutationStatusMap allmutations) throws IOException {
 
     for (final MutationDetails each : allmutations.allMutations()) {
-      final MutationStatusTestPair status = this.thread.getStatus(each.getId());
+      final MutationStatusTestPair status = this.idMap.get(each.getId());
       if (status != null) {
         allmutations.setStatusForMutation(each, status);
       }
@@ -44,10 +49,26 @@ public void results(final MutationStatusMap allmutations) throws IOException {
 
   public ExitCode waitToDie() {
     try {
+      // Wait a moment to give the monitoring thread time to finish naturally. This
+      // happens when the monitored process sends a "DONE" signal over the socket,
+      // the process itself should exit shortly after sending the signal.
+      // Most likely the process will still be running
       Optional<ExitCode> maybeExit = this.thread.waitToFinish(5);
+
+      // While the monitored process reports being alive, keep polling
+      // the monitoring thread to see if it has finished.
       while (!maybeExit.isPresent() && this.process.isAlive()) {
         maybeExit = this.thread.waitToFinish(10);
       }
+
+      // Either the monitored process died, or the thread ended.
+      // Check the thread one last time to try and avoid reporting
+      // an error code if it was the process that went down first
+      maybeExit = this.thread.waitToFinish(10);
+
+      // If the monitored thread is still live, but the process is dead
+      // then either the process never properly started or it died
+      // before reporting its exit
       return maybeExit.orElse(ExitCode.MINION_DIED);
     } finally {
       this.process.destroy();

pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java

@@ -0,0 +1,51 @@
+package org.pitest.mutationtest.execute;
+
+import org.pitest.mutationtest.DetectionStatus;
+import org.pitest.mutationtest.MutationStatusTestPair;
+import org.pitest.mutationtest.engine.MutationIdentifier;
+import org.pitest.util.Id;
+import org.pitest.util.Log;
+import org.pitest.util.ReceiveStrategy;
+import org.pitest.util.SafeDataInputStream;
+
+import java.util.Map;
+import java.util.logging.Logger;
+
+class Receive implements ReceiveStrategy {
+    private static final Logger LOG = Log.getLogger();
+
+    private final Map<MutationIdentifier, MutationStatusTestPair> idMap;
+
+    Receive(final Map<MutationIdentifier, MutationStatusTestPair> idMap) {
+        this.idMap = idMap;
+    }
+
+    @Override
+    public void apply(final byte control, final SafeDataInputStream is) {
+        switch (control) {
+            case Id.DESCRIBE:
+                handleDescribe(is);
+                break;
+            case Id.REPORT:
+                handleReport(is);
+                break;
+            default:
+                LOG.severe("Unknown control byte " + control);
+        }
+    }
+
+    private void handleReport(final SafeDataInputStream is) {
+        final MutationIdentifier mutation = is.read(MutationIdentifier.class);
+        final MutationStatusTestPair value = is
+                .read(MutationStatusTestPair.class);
+        this.idMap.put(mutation, value);
+        LOG.fine(mutation + " " + value);
+    }
+
+    private void handleDescribe(final SafeDataInputStream is) {
+        final MutationIdentifier mutation = is.read(MutationIdentifier.class);
+        this.idMap.put(mutation, MutationStatusTestPair.notAnalysed(1,
+                DetectionStatus.STARTED));
+    }
+
+}

pitest-entry/src/main/java/org/pitest/mutationtest/execute/SendData.java

@@ -0,0 +1,19 @@
+package org.pitest.mutationtest.execute;
+
+import org.pitest.util.SafeDataOutputStream;
+
+import java.util.function.Consumer;
+
+class SendData implements Consumer<SafeDataOutputStream> {
+    private final MinionArguments arguments;
+
+    SendData(final MinionArguments arguments) {
+        this.arguments = arguments;
+    }
+
+    @Override
+    public void accept(final SafeDataOutputStream dos) {
+        dos.write(this.arguments);
+        dos.flush();
+    }
+}

pitest-entry/src/main/java/org/pitest/util/CommunicationThread.java

@@ -25,7 +25,7 @@
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
-public class CommunicationThread {
+public final class CommunicationThread {
 
   private static final Logger                     LOG = Log.getLogger();
 

pitest/src/main/java/org/pitest/mutationtest/execute/MutationTestMinion.java

@@ -99,6 +99,11 @@ public void run() {
               tests, this.reporter));
 
       this.reporter.done(ExitCode.OK);
+
+      // rudely kill the vm in case it is kept alive
+      // by threads launched by client
+      System.exit(0);
+
     } catch (final Throwable ex) {
       ex.printStackTrace(System.out);
       LOG.log(Level.WARNING, "Error during mutation test", ex);