From 495a8ded68a498f11fb27828530628e77a926540 Mon Sep 17 00:00:00 2001 From: Henry Coles Date: Mon, 30 Oct 2023 12:45:05 +0000 Subject: [PATCH 1/3] remove confusing inheritance --- .../execute/CoverageCommunicationThread.java | 19 ---- .../coverage/execute/CoverageProcess.java | 7 +- .../MutationTestCommunicationThread.java | 99 ------------------- .../execute/MutationTestProcess.java | 27 +++-- .../pitest/mutationtest/execute/Receive.java | 49 +++++++++ .../pitest/mutationtest/execute/SendData.java | 19 ++++ .../org/pitest/util/CommunicationThread.java | 2 +- 7 files changed, 94 insertions(+), 128 deletions(-) delete mode 100644 pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageCommunicationThread.java delete mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestCommunicationThread.java create mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java create mode 100644 pitest-entry/src/main/java/org/pitest/mutationtest/execute/SendData.java diff --git a/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageCommunicationThread.java b/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageCommunicationThread.java deleted file mode 100644 index 1e29bd1e6..000000000 --- a/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageCommunicationThread.java +++ /dev/null @@ -1,19 +0,0 @@ -package org.pitest.coverage.execute; - -import java.net.ServerSocket; -import java.util.List; -import java.util.function.Consumer; - -import org.pitest.coverage.CoverageResult; -import org.pitest.util.CommunicationThread; - -public class CoverageCommunicationThread extends CommunicationThread { - - public CoverageCommunicationThread(final ServerSocket socket, - final CoverageOptions arguments, final List tus, - final Consumer handler) { - super(socket, new SendData(arguments, tus), new Receive(handler)); - - } - -} diff --git a/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java b/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java index 1eb127a07..f5f49d7b0 100644 --- a/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java +++ b/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 testClasses, final Consumer 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 { diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestCommunicationThread.java b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestCommunicationThread.java deleted file mode 100644 index 9fa078b0c..000000000 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestCommunicationThread.java +++ /dev/null @@ -1,99 +0,0 @@ -/* - * Copyright 2011 Henry Coles - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and limitations under the License. - */ -package org.pitest.mutationtest.execute; - -import java.net.ServerSocket; -import java.util.Map; -import java.util.function.Consumer; -import java.util.logging.Logger; - -import org.pitest.mutationtest.DetectionStatus; -import org.pitest.mutationtest.MutationStatusTestPair; -import org.pitest.mutationtest.engine.MutationIdentifier; -import org.pitest.util.CommunicationThread; -import org.pitest.util.Id; -import org.pitest.util.Log; -import org.pitest.util.ReceiveStrategy; -import org.pitest.util.SafeDataInputStream; -import org.pitest.util.SafeDataOutputStream; - -public class MutationTestCommunicationThread extends CommunicationThread { - - private static final Logger LOG = Log.getLogger(); - - private static class SendData implements Consumer { - private final MinionArguments arguments; - - SendData(final MinionArguments arguments) { - this.arguments = arguments; - } - - @Override - public void accept(final SafeDataOutputStream dos) { - dos.write(this.arguments); - dos.flush(); - } - } - - private static class Receive implements ReceiveStrategy { - - private final Map idMap; - - Receive(final Map 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; - } - } - - 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)); - } - - } - - private final Map idMap; - - public MutationTestCommunicationThread(final ServerSocket socket, - final MinionArguments arguments, - final Map idMap) { - super(socket, new SendData(arguments), new Receive(idMap)); - this.idMap = idMap; - } - - public MutationStatusTestPair getStatus(final MutationIdentifier id) { - return this.idMap.get(id); - } - -} diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java index f817b76c3..7d921ae63 100644 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java +++ b/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 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,20 @@ 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 Optional 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); } + + // 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(); diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java new file mode 100644 index 000000000..ef17ae79e --- /dev/null +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java @@ -0,0 +1,49 @@ +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 idMap; + + Receive(final Map 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; + } + } + + 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)); + } + +} \ No newline at end of file diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/SendData.java b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/SendData.java new file mode 100644 index 000000000..32213e5be --- /dev/null +++ b/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 { + private final MinionArguments arguments; + + SendData(final MinionArguments arguments) { + this.arguments = arguments; + } + + @Override + public void accept(final SafeDataOutputStream dos) { + dos.write(this.arguments); + dos.flush(); + } +} diff --git a/pitest-entry/src/main/java/org/pitest/util/CommunicationThread.java b/pitest-entry/src/main/java/org/pitest/util/CommunicationThread.java index 64bf3ff8f..08c42e493 100644 --- a/pitest-entry/src/main/java/org/pitest/util/CommunicationThread.java +++ b/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(); From c55ee149523e8e606886657d72dbdbcd34eaefdf Mon Sep 17 00:00:00 2001 From: Henry Coles Date: Mon, 30 Oct 2023 14:05:53 +0000 Subject: [PATCH 2/3] explicitly terminate jvm in mutation minion --- .../pitest/mutationtest/execute/Receive.java | 2 + .../execute/MutationTestMinion.java | 5 + .../execute/MutationTestMinionTest.java | 102 ------------------ 3 files changed, 7 insertions(+), 102 deletions(-) delete mode 100644 pitest/src/test/java/org/pitest/mutationtest/execute/MutationTestMinionTest.java diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java index ef17ae79e..e6c62c083 100644 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/Receive.java @@ -29,6 +29,8 @@ public void apply(final byte control, final SafeDataInputStream is) { case Id.REPORT: handleReport(is); break; + default: + LOG.severe("Unknown control byte " + control); } } diff --git a/pitest/src/main/java/org/pitest/mutationtest/execute/MutationTestMinion.java b/pitest/src/main/java/org/pitest/mutationtest/execute/MutationTestMinion.java index 5322f181b..524f4c6a9 100644 --- a/pitest/src/main/java/org/pitest/mutationtest/execute/MutationTestMinion.java +++ b/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); diff --git a/pitest/src/test/java/org/pitest/mutationtest/execute/MutationTestMinionTest.java b/pitest/src/test/java/org/pitest/mutationtest/execute/MutationTestMinionTest.java deleted file mode 100644 index 0e0dcfc9b..000000000 --- a/pitest/src/test/java/org/pitest/mutationtest/execute/MutationTestMinionTest.java +++ /dev/null @@ -1,102 +0,0 @@ -package org.pitest.mutationtest.execute; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.atLeastOnce; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import static org.pitest.mutationtest.LocationMother.aMutationId; - -import java.util.ArrayList; -import java.util.Collection; - -import org.junit.Before; -import org.junit.Test; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.MockitoAnnotations; -import org.pitest.classinfo.ClassByteArraySource; -import org.pitest.classinfo.ClassName; -import org.pitest.mutationtest.EngineArguments; -import org.pitest.mutationtest.MutationEngineFactory; -import org.pitest.mutationtest.TimeoutLengthStrategy; -import org.pitest.mutationtest.config.MinionSettings; -import org.pitest.mutationtest.config.TestPluginArguments; -import org.pitest.mutationtest.engine.Mutater; -import org.pitest.mutationtest.engine.MutationDetails; -import org.pitest.mutationtest.engine.MutationEngine; -import org.pitest.mutationtest.engine.MutationIdentifier; -import org.pitest.testapi.Configuration; -import org.pitest.util.ExitCode; -import org.pitest.util.PitError; -import org.pitest.util.SafeDataInputStream; -import org.pitest.util.Verbosity; - -public class MutationTestMinionTest { - - private MutationTestMinion testee; - - @Mock - private Reporter reporter; - - @Mock - private SafeDataInputStream is; - - @Mock - private MutationEngine engine; - - @Mock - private TimeoutLengthStrategy timeoutStrategy; - - @Mock - private Configuration testConfig; - - @Mock - private Mutater mutater; - - @Mock - private MinionSettings settings; - - private MinionArguments args; - - private Collection mutations; - - private Collection tests; - - @Before - public void setup() { - MockitoAnnotations.openMocks(this); - this.mutations = new ArrayList<>(); - this.tests = new ArrayList<>(); - - this.args = new MinionArguments(this.mutations, this.tests, "anEgine", EngineArguments.arguments(), - this.timeoutStrategy, Verbosity.DEFAULT, false, TestPluginArguments.defaults()); - - when(this.is.read(MinionArguments.class)).thenReturn(this.args); - when(this.engine.createMutator(any(ClassByteArraySource.class))) - .thenReturn(this.mutater); - - final MutationEngineFactory factory = Mockito.mock(MutationEngineFactory.class); - when(factory.createEngine(any(EngineArguments.class))).thenReturn(this.engine); - - when(this.settings.createEngine(any(String.class))).thenReturn(factory); - - this.testee = new MutationTestMinion(this.settings, this.is, this.reporter); - } - - @Test - public void shouldReportNoErrorWhenNoMutationsSupplied() { - this.testee.run(); - verify(this.reporter, atLeastOnce()).done(ExitCode.OK); - } - - @Test - public void shouldReportErrorWhenOneOccursDuringAnalysis() { - this.mutations.add(new MutationDetails(aMutationId().withIndex(0) - .withMutator("foo").build(), "file", "desc", 0, 0)); - when(this.mutater.getMutation(any(MutationIdentifier.class))).thenThrow( - new PitError("foo")); - this.testee.run(); - verify(this.reporter).done(ExitCode.UNKNOWN_ERROR); - } - -} From 56ca9e8cf1ea428bec54248833a00f7dc5d7d133 Mon Sep 17 00:00:00 2001 From: Henry Coles Date: Tue, 31 Oct 2023 10:28:55 +0000 Subject: [PATCH 3/3] recheck thread exit code before reporting doa minion --- .../java/org/pitest/coverage/execute/CoverageProcess.java | 6 ++++++ .../pitest/mutationtest/execute/MutationTestProcess.java | 8 +++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java b/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java index f5f49d7b0..22a9d863c 100644 --- a/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java +++ b/pitest-entry/src/main/java/org/pitest/coverage/execute/CoverageProcess.java @@ -37,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(); diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java index 7d921ae63..27b0d50e2 100644 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/execute/MutationTestProcess.java @@ -51,7 +51,8 @@ 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 + // the process itself should exit shortly after sending the signal. + // Most likely the process will still be running Optional maybeExit = this.thread.waitToFinish(5); // While the monitored process reports being alive, keep polling @@ -60,6 +61,11 @@ public ExitCode waitToDie() { 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