Bug 567345 - Await termination descendants of RuntimeProcess

and stream-line RuntimeProcess.terminate()

Change-Id: If90808ff6fc8bb0adaa23f97cf3e879a61bcb6dc
Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>

4 files changed, 135 insertions, 36 deletions

diff --git a/org.eclipse.debug.core/core/org/eclipse/debug/core/model/RuntimeProcess.java b/org.eclipse.debug.core/core/org/eclipse/debug/core/model/RuntimeProcess.java
index 51186f956..618b5cf64 100644
--- a/org.eclipse.debug.core/core/org/eclipse/debug/core/model/RuntimeProcess.java
+++ b/org.eclipse.debug.core/core/org/eclipse/debug/core/model/RuntimeProcess.java
@@ -21,7 +21,9 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.stream.Collectors;
 
 import org.eclipse.core.runtime.IStatus;
@@ -52,8 +54,7 @@ import org.eclipse.debug.internal.core.StreamsProxy;
  */
 public class RuntimeProcess extends PlatformObject implements IProcess {
 
-	private static final int MAX_WAIT_FOR_DEATH_ATTEMPTS = 10;
-	private static final int TIME_TO_WAIT_FOR_THREAD_DEATH = 500; // ms
+	private static final int TERMINATION_TIMEOUT = 5000; // ms
 
 	/**
 	 * The launch this process is contained in
@@ -207,39 +208,34 @@ public class RuntimeProcess extends PlatformObject implements IProcess {
 				((StreamsProxy) fStreamsProxy).kill();
 			}
 			Process process = getSystemProcess();
-			if (process != null) {
-
-				List<ProcessHandle> descendants; // only a snapshot!
-				try {
-					descendants = process.descendants().collect(Collectors.toList());
-				} catch (UnsupportedOperationException e) {
-					// JVM may not support toHandle() -> assume no descendants
-					descendants = Collections.emptyList();
-				}
+			if (process == null) {
+				return;
+			}
 
-				process.destroy();
-				descendants.forEach(ProcessHandle::destroy);
+			List<ProcessHandle> descendants; // only a snapshot!
+			try {
+				descendants = process.descendants().collect(Collectors.toList());
+			} catch (UnsupportedOperationException e) {
+				// JVM may not support toHandle() -> assume no descendants
+				descendants = Collections.emptyList();
 			}
-			int attempts = 0;
-			while (attempts < MAX_WAIT_FOR_DEATH_ATTEMPTS) {
-				try {
-					process = getSystemProcess();
-					if (process != null) {
-						fExitValue = process.exitValue(); // throws exception if process not exited
-					}
-					return;
-				} catch (IllegalThreadStateException ie) {
-				}
-				try {
-					if (process != null) {
-						process.waitFor(TIME_TO_WAIT_FOR_THREAD_DEATH, TimeUnit.MILLISECONDS);
-					} else {
-						Thread.sleep(TIME_TO_WAIT_FOR_THREAD_DEATH);
+
+			process.destroy();
+			descendants.forEach(ProcessHandle::destroy);
+
+			// await termination of process and descendants
+			try { // (in total don't wait longer than TERMINATION_TIMEOUT)
+				long waitStart = System.currentTimeMillis();
+				if (process.waitFor(TERMINATION_TIMEOUT, TimeUnit.MILLISECONDS)) {
+					fExitValue = process.exitValue();
+					if (waitFor(descendants, waitStart)) {
+						return;
 					}
-				} catch (InterruptedException e) {
 				}
-				attempts++;
+			} catch (InterruptedException e) {
+				Thread.currentThread().interrupt();
 			}
+
 			// clean-up
 			if (fMonitor != null) {
 				fMonitor.killThread();
@@ -251,6 +247,36 @@ public class RuntimeProcess extends PlatformObject implements IProcess {
 	}
 
 	/**
+	 * Awaits the termination of the processes of the given ProcessHandles.
+	 * <p>
+	 * If all of the specified processes terminate before {@code waitStart} +
+	 * {@link #TERMINATION_TIMEOUT} this methods returns {@code true}. If any
+	 * process has not terminated until the so specified timeout this methods
+	 * aborts waiting and returns {@code false}.
+	 * </p>
+	 *
+	 * @param descendants the list of handles to the processes to await
+	 * @param waitStart the time when await of the process termination started
+	 * @return true if each process has terminated (before timeout), else false
+	 * @throws InterruptedException if the current thread was interrupted while
+	 *             waiting
+	 */
+	private boolean waitFor(List<ProcessHandle> descendants, long waitStart) throws InterruptedException {
+		try {
+			for (ProcessHandle handle : descendants) {
+				long remainingTime = TERMINATION_TIMEOUT - (System.currentTimeMillis() - waitStart);
+				// await termination of this descendant
+				handle.onExit().get(remainingTime, TimeUnit.MILLISECONDS);
+			}
+			return true;
+		} catch (ExecutionException e) { // should not happen
+			throw new IllegalStateException(e.getCause());
+		} catch (TimeoutException e) {
+			return false; // any sub-processes timed out
+		}
+	}
+
+	/**
 	 * Notification that the system process associated with this process
 	 * has terminated.
 	 */
diff --git a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcess.java b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcess.java
index bac6a22d0..21f388053 100644
--- a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcess.java
+++ b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcess.java
@@ -69,6 +69,9 @@ public class MockProcess extends Process {
 	/** The child/sub mock-processes of this mock-process. */
 	private Optional<MockProcessHandle> handle = Optional.of(new MockProcessHandle(this));
 
+	/** The delay after a call to destroy() until actual termination. */
+	private int terminationDelay = 0;
+
 	/**
 	 * Create new silent mockup process which runs for a given amount of time.
 	 * Does not read input or produce any output.
@@ -191,6 +194,7 @@ public class MockProcess extends Process {
 				}
 			}
 		}
+		handle.ifPresent(MockProcessHandle::setTerminated);
 		return exitCode;
 	}
 
@@ -208,6 +212,9 @@ public class MockProcess extends Process {
 				remainingMs = timeoutMs - System.currentTimeMillis();
 			}
 		}
+		if (isTerminated()) {
+			handle.ifPresent(MockProcessHandle::setTerminated);
+		}
 		return isTerminated();
 	}
 
@@ -222,7 +229,7 @@ public class MockProcess extends Process {
 
 	@Override
 	public void destroy() {
-		destroy(0);
+		destroy(terminationDelay);
 	}
 
 	/**
@@ -235,6 +242,9 @@ public class MockProcess extends Process {
 		synchronized (waitForTerminationLock) {
 			endTime = System.currentTimeMillis() + delay;
 			waitForTerminationLock.notifyAll();
+			if (delay <= 0) {
+				handle.ifPresent(MockProcessHandle::setTerminated);
+			}
 		}
 	}
 
@@ -267,6 +277,16 @@ public class MockProcess extends Process {
 	}
 
 	/**
+	 * Set the delay between a call to destroy and the termination of this
+	 * process.
+	 *
+	 * @param delay the delay after a call to destroy() until actual termination
+	 */
+	public void setTerminationDelay(int delay) {
+		this.terminationDelay = delay;
+	}
+
+	/**
 	 * Create a {@link RuntimeProcess} which wraps this {@link MockProcess}.
 	 * <p>
 	 * Note: the process will only be connected to a minimal dummy launch
diff --git a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcessHandle.java b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcessHandle.java
index 50841a660..a9f0b5609 100644
--- a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcessHandle.java
+++ b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/MockProcessHandle.java
@@ -28,6 +28,7 @@ public class MockProcessHandle implements ProcessHandle {
 
 	private final MockProcess process;
 	private final Collection<ProcessHandle> children;
+	private final CompletableFuture<ProcessHandle> onExit = new CompletableFuture<>();
 
 	/**
 	 * Create new mockup process handle for a process without children.
@@ -60,6 +61,11 @@ public class MockProcessHandle implements ProcessHandle {
 	}
 
 	@Override
+	public CompletableFuture<ProcessHandle> onExit() {
+		return onExit; // onExit must be completed by process
+	}
+
+	@Override
 	public boolean supportsNormalTermination() {
 		return true;
 	}
@@ -85,6 +91,15 @@ public class MockProcessHandle implements ProcessHandle {
 		return Long.compare(pid(), ((MockProcessHandle) other).pid());
 	}
 
+	/**
+	 * Notify this handle that this mock-process has terminated. If this is not
+	 * called handles of descendant processes do not know about the termination
+	 * (and for descendants only the handle is queried, not the process itself).
+	 */
+	void setTerminated() {
+		onExit.complete(this);
+	}
+
 	// not yet implemented methods
 
 	@Override
@@ -101,9 +116,4 @@ public class MockProcessHandle implements ProcessHandle {
 	public Info info() {
 		throw new UnsupportedOperationException("Not yet implemented");
 	}
-
-	@Override
-	public CompletableFuture<ProcessHandle> onExit() {
-		throw new UnsupportedOperationException("Not yet implemented");
-	}
 }
diff --git a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/RuntimeProcessTests.java b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/RuntimeProcessTests.java
index 7dbdd2215..79e4b68cd 100644
--- a/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/RuntimeProcessTests.java
+++ b/org.eclipse.debug.tests/src/org/eclipse/debug/tests/console/RuntimeProcessTests.java
@@ -10,9 +10,12 @@
  *
  * Contributors:
  *     Paul Pazderski - initial API and implementation
+ *     Hannes Wellmann - add tests regarding termination of descendants and timeout
  *******************************************************************************/
 package org.eclipse.debug.tests.console;
 
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThrows;
@@ -22,11 +25,14 @@ import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.eclipse.debug.core.DebugEvent;
+import org.eclipse.debug.core.DebugException;
 import org.eclipse.debug.core.DebugPlugin;
 import org.eclipse.debug.core.model.RuntimeProcess;
+import org.eclipse.debug.internal.core.DebugCoreMessages;
 import org.eclipse.debug.tests.AbstractDebugTest;
 import org.eclipse.debug.tests.TestUtil;
 import org.junit.Test;
+import org.junit.function.ThrowingRunnable;
 
 public class RuntimeProcessTests extends AbstractDebugTest {
 
@@ -140,4 +146,41 @@ public class RuntimeProcessTests extends AbstractDebugTest {
 
 		TestUtil.waitWhile(p -> !p.isTerminated(), runtimeProcess, 1000, p -> "RuntimePocess not terminated.");
 	}
+
+	/**
+	 * Test {@link RuntimeProcess} terminating the wrapped process which does
+	 * only terminate with a delay.
+	 */
+	@Test
+	public void testTerminateProcessWithTimeoutExeedingTermination() {
+
+		MockProcess mockProcess = new MockProcess(MockProcess.RUN_FOREVER);
+		mockProcess.setTerminationDelay(6000);
+
+		RuntimeProcess runtimeProcess = mockProcess.toRuntimeProcess();
+		ThrowingRunnable termianteProcess = () -> runtimeProcess.terminate();
+
+		DebugException timeoutException = assertThrows(DebugException.class, termianteProcess);
+		assertThat(timeoutException.getMessage(), is(DebugCoreMessages.RuntimeProcess_terminate_failed));
+	}
+
+	/**
+	 * Test {@link RuntimeProcess} terminating the wrapped process which does
+	 * only terminate with a delay.
+	 */
+	@Test
+	public void testTerminateProcessWithDescendentExceedingTimeoutForTermination() {
+
+		MockProcess childProcess = new MockProcess(MockProcess.RUN_FOREVER);
+		childProcess.setTerminationDelay(6000);
+
+		MockProcess mockProcess = new MockProcess(MockProcess.RUN_FOREVER);
+		mockProcess.setHandle(new MockProcessHandle(mockProcess, List.of(childProcess)));
+
+		RuntimeProcess runtimeProcess = mockProcess.toRuntimeProcess();
+		ThrowingRunnable termianteProcess = () -> runtimeProcess.terminate();
+
+		DebugException timeoutException = assertThrows(DebugException.class, termianteProcess);
+		assertThat(timeoutException.getMessage(), is(DebugCoreMessages.RuntimeProcess_terminate_failed));
+	}
 }