Bug 576945: StreamsProxy needs to be closed after app exitI20211104-1800

Capture and forward all output from application by closing the
StreamsProxy after the graceful exit timeout.

Contributed by STMicroelectronics

Change-Id: I139e414ee0b617085cbe66df3d788e73f1b50e78
Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@st.com>
Reviewed-on: https://git.eclipse.org/r/c/platform/eclipse.platform.debug/+/187134
Tested-by: Platform Bot <platform-bot@eclipse.org>
Reviewed-by: Jonah Graham <jonah@kichwacoders.com>

3 files changed, 130 insertions, 29 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 98122a8d6..35f17f8c6 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
@@ -216,37 +216,41 @@ public class RuntimeProcess extends PlatformObject implements IProcess {
 	@Override
 	public void terminate() throws DebugException {
 		if (!isTerminated()) {
-			if (fStreamsProxy instanceof StreamsProxy) {
-				((StreamsProxy) fStreamsProxy).kill();
-			}
-			Process process = getSystemProcess();
-			if (process == null) {
-				return;
-			}
-
-			List<ProcessHandle> descendants = Collections.emptyList();
-			if (fTerminateDescendants) {
-				try { // List of descendants of process is only a snapshot!
-					descendants = process.descendants().collect(Collectors.toList());
-				} catch (UnsupportedOperationException e) {
-					// JVM may not support toHandle() -> assume no descendants
+			try {
+				Process process = getSystemProcess();
+				if (process == null) {
+					return;
 				}
-			}
 
-			process.destroy();
-			descendants.forEach(ProcessHandle::destroy);
+				List<ProcessHandle> descendants = Collections.emptyList();
+				if (fTerminateDescendants) {
+					try { // List of descendants of process is only a snapshot!
+						descendants = process.descendants().collect(Collectors.toList());
+					} catch (UnsupportedOperationException e) {
+						// JVM may not support toHandle() -> assume no
+						// descendants
+					}
+				}
 
-			// 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;
+				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) {
+					Thread.currentThread().interrupt();
+				}
+			} finally {
+				if (fStreamsProxy instanceof StreamsProxy) {
+					((StreamsProxy) fStreamsProxy).kill();
 				}
-			} catch (InterruptedException e) {
-				Thread.currentThread().interrupt();
 			}
 
 			// clean-up
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 21f388053..992a0c3a6 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
@@ -15,6 +15,7 @@ package org.eclipse.debug.tests.console;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.Map;
@@ -73,6 +74,31 @@ public class MockProcess extends Process {
 	private int terminationDelay = 0;
 
 	/**
+	 * For mode used by constructor {@link MockProcess#MockProcess()} this
+	 * indicates via stdout what the state of the process is.
+	 */
+	public static enum ProcessState {
+		RUNNING('R'), DESTROYING('D'),
+		/**
+		 * Last read is special as it should only be returned once to ensure
+		 * that we get the last character on the stream.
+		 */
+		LASTREAD('L'), TERMINATED(-1);
+
+		private int code;
+
+		ProcessState(int c) {
+			this.code = c;
+		}
+
+		public int getCode() {
+			return code;
+		}
+	}
+
+	private volatile ProcessState processState = ProcessState.RUNNING;
+
+	/**
 	 * Create new silent mockup process which runs for a given amount of time.
 	 * Does not read input or produce any output.
 	 *
@@ -141,6 +167,34 @@ public class MockProcess extends Process {
 	}
 
 	/**
+	 * Create a new mock process that the stdout stream will indicate status of
+	 * the process. The codes are defined by {@link ProcessState#getCode()}
+	 */
+	public MockProcess() {
+		super();
+		this.stderr = new ByteArrayInputStream(new byte[0]);
+		this.endTime = RUN_FOREVER;
+		this.stdout = new InputStream() {
+			@Override
+			public int read() throws IOException {
+				if (processState == ProcessState.LASTREAD) {
+
+					// Uncomment this sleep and the test will fail because
+					// RuntimeProcess.terminate does not wait until
+					// the monitor threads complete.
+					// try {
+					// Thread.sleep(1000);
+					// } catch (InterruptedException e) {
+					// }
+					processState = ProcessState.TERMINATED;
+					return ProcessState.LASTREAD.getCode();
+				}
+				return processState.getCode();
+			}
+		};
+	}
+
+	/**
 	 * Get bytes received through stdin since last invocation of this method.
 	 * <p>
 	 * Not thread safe. It may miss some input if new content is written while
@@ -194,7 +248,7 @@ public class MockProcess extends Process {
 				}
 			}
 		}
-		handle.ifPresent(MockProcessHandle::setTerminated);
+		setTerminated();
 		return exitCode;
 	}
 
@@ -213,7 +267,7 @@ public class MockProcess extends Process {
 			}
 		}
 		if (isTerminated()) {
-			handle.ifPresent(MockProcessHandle::setTerminated);
+			setTerminated();
 		}
 		return isTerminated();
 	}
@@ -239,11 +293,12 @@ public class MockProcess extends Process {
 	 *            and before the mockup process goes in terminated state
 	 */
 	public void destroy(int delay) {
+		processState = ProcessState.DESTROYING;
 		synchronized (waitForTerminationLock) {
 			endTime = System.currentTimeMillis() + delay;
 			waitForTerminationLock.notifyAll();
 			if (delay <= 0) {
-				handle.ifPresent(MockProcessHandle::setTerminated);
+				setTerminated();
 			}
 		}
 	}
@@ -333,4 +388,16 @@ public class MockProcess extends Process {
 		}
 		return (RuntimeProcess) DebugPlugin.newProcess(new Launch(launchConfiguration, ILaunchManager.RUN_MODE, null), this, name);
 	}
+
+	/**
+	 * Move state machines to terminated.
+	 */
+	private void setTerminated() {
+		synchronized (this) {
+			if (processState != ProcessState.TERMINATED) {
+				processState = ProcessState.LASTREAD;
+			}
+		}
+		handle.ifPresent(MockProcessHandle::setTerminated);
+	}
 }
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 bbcca06ea..c29a24984 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
@@ -21,17 +21,22 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
+import java.io.InputStream;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import org.eclipse.core.runtime.IStatus;
 import org.eclipse.debug.core.DebugEvent;
 import org.eclipse.debug.core.DebugException;
 import org.eclipse.debug.core.DebugPlugin;
+import org.eclipse.debug.core.model.IProcess;
 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.eclipse.debug.tests.sourcelookup.TestLaunch;
 import org.junit.Test;
 
 public class RuntimeProcessTests extends AbstractDebugTest {
@@ -206,4 +211,29 @@ public class RuntimeProcessTests extends AbstractDebugTest {
 		DebugException timeoutException = assertThrows(DebugException.class, runtimeProcess::terminate);
 		assertThat(timeoutException.getMessage(), is(DebugCoreMessages.RuntimeProcess_terminate_failed));
 	}
+
+	@Test
+	public void testOutputAfterDestroy() throws Exception {
+		MockProcess proc = new MockProcess();
+
+		IProcess iProc = new RuntimeProcess(new TestLaunch(), proc, "foo", Collections.emptyMap());
+		iProc.terminate();
+
+		String str = iProc.getStreamsProxy().getOutputStreamMonitor().getContents();
+		TestUtil.log(IStatus.INFO, name.getMethodName(), "Stream result: ");
+		for (int i = 0; i < str.length(); i += 100) {
+			TestUtil.log(IStatus.INFO, name.getMethodName(), str.substring(i, Math.min(i + 100, str.length())));
+		}
+		TestUtil.log(IStatus.INFO, name.getMethodName(), "Stream done.");
+		// Make sure that the inputstream (process's stdout) has been fully read
+		// and is at EOF
+		@SuppressWarnings("resource")
+		InputStream inputStream = proc.getInputStream();
+		assertEquals(-1, inputStream.read());
+		// Make sure that the last character in the stream makes it through to
+		// the monitor
+		assertTrue(str.endsWith(String.valueOf((char) MockProcess.ProcessState.LASTREAD.getCode())));
+	}
+
+
 }