Target Explorer: Fix value-add may hang up as stdout and stderr of the value add process are not read anymore once launched

3 files changed, 68 insertions, 27 deletions

diff --git a/target_explorer/plugins/org.eclipse.tcf.te.runtime/src/org/eclipse/tcf/te/runtime/processes/ProcessOutputReaderThread.java b/target_explorer/plugins/org.eclipse.tcf.te.runtime/src/org/eclipse/tcf/te/runtime/processes/ProcessOutputReaderThread.java
index 21d3767d4..09ff5f98d 100644
--- a/target_explorer/plugins/org.eclipse.tcf.te.runtime/src/org/eclipse/tcf/te/runtime/processes/ProcessOutputReaderThread.java
+++ b/target_explorer/plugins/org.eclipse.tcf.te.runtime/src/org/eclipse/tcf/te/runtime/processes/ProcessOutputReaderThread.java
@@ -35,6 +35,7 @@ public class ProcessOutputReaderThread extends Thread {
 	// String builder to collect the read lines
 	private StringBuilder lines;
 	private String lastLine;
+	private boolean buffering;
 
 	// finished reading all the output
 	private boolean finished;
@@ -54,6 +55,7 @@ public class ProcessOutputReaderThread extends Thread {
 		assert streams != null;
 
 		lastLine = ""; //$NON-NLS-1$
+		buffering = true;
 		finished = false;
 		waiting = false;
 		waiterSemaphore = new Object();
@@ -78,6 +80,28 @@ public class ProcessOutputReaderThread extends Thread {
 	}
 
 	/**
+	 * Returns if or if not the process output reader is buffering the output read.
+	 *
+	 * @return <code>True</code> if the output reader is buffering the output read, <code>false</code> otherwise.
+	 */
+	public final boolean isBuffering() {
+		return buffering;
+	}
+
+	/**
+	 * Toggle if or if not the process output reader is buffering the output read.
+	 * <p>
+	 * <b>Note:</b> Switching off the buffering will reset the buffer too. Read the
+	 * output buffer before switching off the buffering if needed.
+	 *
+	 * @param buffering <code>True</code> if the output reader shall buffer the output read, <code>false</code> otherwise.
+	 */
+	public final void setBuffering(boolean buffering) {
+		this.buffering = buffering;
+		if (!buffering) lines = new StringBuilder();
+	}
+
+	/**
 	 * Returns if or if not the process output reader thread has finished.
 	 *
 	 * @return <code>true</code> if the thread is finished, <code>false</code> otherwise.
@@ -142,8 +166,10 @@ public class ProcessOutputReaderThread extends Thread {
 			}
 			line = buffer.toString();
 			lastLine = line;
-			lines.append(line);
-			lines.append('\n');
+			if (buffering) {
+				lines.append(line);
+				lines.append('\n');
+			}
 			CoreBundleActivator.getTraceHandler().trace(getPrefix() + " processLine: " + line, 3, this); //$NON-NLS-1$
 		}
 	}
diff --git a/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/AbstractExternalValueAdd.java b/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/AbstractExternalValueAdd.java
index 0924d9f86..3a565a078 100644
--- a/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/AbstractExternalValueAdd.java
+++ b/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/AbstractExternalValueAdd.java
@@ -25,7 +25,9 @@ import org.eclipse.tcf.protocol.IPeer;
 import org.eclipse.tcf.protocol.JSON;
 import org.eclipse.tcf.protocol.Protocol;
 import org.eclipse.tcf.te.runtime.callback.Callback;
+import org.eclipse.tcf.te.runtime.interfaces.IDisposable;
 import org.eclipse.tcf.te.runtime.interfaces.callback.ICallback;
+import org.eclipse.tcf.te.runtime.processes.ProcessOutputReaderThread;
 import org.eclipse.tcf.te.runtime.utils.net.IPAddressUtil;
 import org.eclipse.tcf.te.tcf.core.activator.CoreBundleActivator;
 import org.eclipse.tcf.te.tcf.core.interfaces.tracing.ITraceIds;
@@ -42,9 +44,21 @@ public abstract class AbstractExternalValueAdd extends AbstractValueAdd {
 	/**
 	 * Class representing a value add entry
 	 */
-	protected static class ValueAddEntry {
+	protected static class ValueAddEntry implements IDisposable {
 		public Process process;
 		public IPeer peer;
+		public ProcessOutputReaderThread outputReader;
+		public ProcessOutputReaderThread errorReader;
+
+		/* (non-Javadoc)
+		 * @see org.eclipse.tcf.te.runtime.interfaces.IDisposable#dispose()
+		 */
+		@Override
+		public void dispose() {
+			if (process != null) process.destroy();
+			if (outputReader != null) outputReader.interrupt();
+			if (errorReader != null) errorReader.interrupt();
+		}
 	}
 
 	/* (non-Javadoc)
@@ -140,9 +154,7 @@ public abstract class AbstractExternalValueAdd extends AbstractValueAdd {
 						channel.removeChannelListener(this);
 						// External value-add is not longer alive, clean up
 						entries.remove(id);
-						if (finEntry.process != null) {
-							finEntry.process.destroy();
-						}
+						finEntry.dispose();
 						// Invoke the callback
 						done.done(AbstractExternalValueAdd.this, Status.OK_STATUS);
 					}
@@ -199,6 +211,9 @@ public abstract class AbstractExternalValueAdd extends AbstractValueAdd {
 				} catch (IllegalThreadStateException e) {
 					// Still running -> Associate the process with the entry
 					entry.process = process;
+					// Associate the reader threads with the entry
+					entry.outputReader = launcher.getOutputReader();
+					entry.errorReader = launcher.getErrorReader();
 				}
 			}
 
@@ -286,9 +301,9 @@ public abstract class AbstractExternalValueAdd extends AbstractValueAdd {
 				entries.put(id, entry);
 			}
 
-			// Stop the output reader thread
+			// Stop the buffering of the output reader
 			if (launcher.getOutputReader() != null) {
-				launcher.getOutputReader().interrupt();
+				launcher.getOutputReader().setBuffering(false);
 			}
 		} else {
 			error = new FileNotFoundException(NLS.bind(Messages.AbstractExternalValueAdd_error_invalidLocation, this.getId()));
@@ -343,9 +358,7 @@ public abstract class AbstractExternalValueAdd extends AbstractValueAdd {
 					boolean alive = ((Boolean)getResult()).booleanValue();
 					if (alive) {
 						entries.remove(id);
-						if (entry.process != null) {
-							entry.process.destroy();
-						}
+						entry.dispose();
 					}
 					done.done(AbstractExternalValueAdd.this, Status.OK_STATUS);
 				}
@@ -367,9 +380,7 @@ public abstract class AbstractExternalValueAdd extends AbstractValueAdd {
 		// We force the value-add to shutdown if not yet gone by destroying the process.
 		for (Entry<String, ValueAddEntry> entry : entries.entrySet()) {
 			ValueAddEntry value = entry.getValue();
-			if (value.process != null) {
-				value.process.destroy();
-			}
+			value.dispose();
 		}
 		// Clear all entries from the list
 		entries.clear();
diff --git a/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/ValueAddLauncher.java b/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/ValueAddLauncher.java
index e986fc212..c886dfc55 100644
--- a/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/ValueAddLauncher.java
+++ b/target_explorer/plugins/org.eclipse.tcf.te.tcf.core/src/org/eclipse/tcf/te/tcf/core/va/ValueAddLauncher.java
@@ -20,7 +20,6 @@ import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.Platform;
 import org.eclipse.osgi.util.NLS;
-import org.eclipse.tcf.te.runtime.interfaces.IDisposable;
 import org.eclipse.tcf.te.runtime.processes.ProcessLauncher;
 import org.eclipse.tcf.te.runtime.processes.ProcessOutputReaderThread;
 import org.eclipse.tcf.te.runtime.utils.Host;
@@ -32,7 +31,7 @@ import org.osgi.framework.Bundle;
 /**
  * Value-add launcher implementation.
  */
-public class ValueAddLauncher extends ProcessLauncher implements IDisposable {
+public class ValueAddLauncher extends ProcessLauncher {
 	// The target peer id
 	private final String id;
 	// The path of the value-add to launch
@@ -43,6 +42,8 @@ public class ValueAddLauncher extends ProcessLauncher implements IDisposable {
 	private Process process;
 	// The process output reader
 	private ProcessOutputReaderThread outputReader;
+	// The process error reader
+	private ProcessOutputReaderThread errorReader;
 
 	/**
 	 * Constructor.
@@ -62,17 +63,6 @@ public class ValueAddLauncher extends ProcessLauncher implements IDisposable {
 		this.valueAddId = valueAddId;
 	}
 
-	/* (non-Javadoc)
-	 * @see org.eclipse.tcf.te.runtime.interfaces.IDisposable#dispose()
-	 */
-	@Override
-	public void dispose() {
-		if (process != null) {
-			process.destroy();
-			process = null;
-		}
-	}
-
 	/**
 	 * Returns the process handle.
 	 *
@@ -91,6 +81,15 @@ public class ValueAddLauncher extends ProcessLauncher implements IDisposable {
 		return outputReader;
 	}
 
+	/**
+	 * Returns the process error reader.
+	 *
+	 * @return The process error reader or <code>null</code>.
+	 */
+	public ProcessOutputReaderThread getErrorReader() {
+		return errorReader;
+	}
+
 	/* (non-Javadoc)
 	 * @see org.eclipse.tcf.te.runtime.processes.ProcessLauncher#launch()
 	 */
@@ -143,6 +142,11 @@ public class ValueAddLauncher extends ProcessLauncher implements IDisposable {
 		// Launch the process output reader
 		outputReader = new ProcessOutputReaderThread(path.lastSegment(), new InputStream[] { process.getInputStream() });
 		outputReader.start();
+
+		// Launch the process error reader (not buffering)
+		errorReader = new ProcessOutputReaderThread(path.lastSegment(), new InputStream[] { process.getErrorStream() });
+		errorReader.setBuffering(false);
+		errorReader.start();
 	}
 
 	/**