Fix deadlock in StreamGobbler

Also,
- Remove dead code.
- Fix spelling and formatting.
- Use a predictable test input stream in StreamGobblerTest.
- Rename reader thread from to to reader.

Change-Id: I94b972874a56cbab116ac60faab7e9767621b79f
Reviewed-on: https://git.eclipse.org/r/8453
Tested-by: Hudson CI
Reviewed-by: Roland Grunberg <rgrunber@redhat.com>
IP-Clean: Roland Grunberg <rgrunber@redhat.com>
Tested-by: Roland Grunberg <rgrunber@redhat.com>
Reviewed-on: https://git.eclipse.org/r/8702
Reviewed-by: Jeff Johnston <jjohnstn@redhat.com>
IP-Clean: Jeff Johnston <jjohnstn@redhat.com>
Tested-by: Jeff Johnston <jjohnstn@redhat.com>

4 files changed, 67 insertions, 89 deletions

diff --git a/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/LoggedCommand2.java b/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/LoggedCommand2.java
index 225a941ce6..9cf66a765c 100644
--- a/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/LoggedCommand2.java
+++ b/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/LoggedCommand2.java
@@ -79,9 +79,8 @@ public class LoggedCommand2 extends ScpExec {
 	@Override
 	public synchronized void stop() {
 		if(isRunning()) {
-	    	removeInputStreamListener(logger);
-	    	super.stop();
-	    	
+			super.stop();
+			removeInputStreamListener(logger);
 		}
 	}
 	
diff --git a/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/structures/ScriptConsole.java b/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/structures/ScriptConsole.java
index 17c428f1c7..969b5a8ec3 100644
--- a/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/structures/ScriptConsole.java
+++ b/systemtap/org.eclipse.linuxtools.systemtap.ui.consolelog/src/org/eclipse/linuxtools/systemtap/ui/consolelog/structures/ScriptConsole.java
@@ -255,10 +255,6 @@ public class ScriptConsole extends IOConsole {
               cmd.removeInputStreamListener(consoleDaemon);
               setName(Localization.getString("ScriptConsole.Terminated") + super.getName());
 			ConsolePlugin.getDefault().getConsoleManager().removeConsoles(new IConsole[] {this});
-			
-	        
-			
-			
 		}
 	}
 	
diff --git a/systemtap/org.eclipse.linuxtools.systemtap.ui.structures.tests/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobblerTest.java b/systemtap/org.eclipse.linuxtools.systemtap.ui.structures.tests/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobblerTest.java
index db625d0fc3..8cbcf99bb0 100644
--- a/systemtap/org.eclipse.linuxtools.systemtap.ui.structures.tests/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobblerTest.java
+++ b/systemtap/org.eclipse.linuxtools.systemtap.ui.structures.tests/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobblerTest.java
@@ -15,14 +15,27 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
+import java.io.InputStream;
+
 import org.junit.Before;
 import org.junit.Test;
 
 public class StreamGobblerTest{
 
+	private static class TestStream extends InputStream{
+		int i = 10;
+		@Override
+		public int read() {
+			if (i < 0)
+				return -1;
+
+			return i--;
+		}
+	}
+
 	@Before
 	public void setUp() {
-		sg = new StreamGobbler(System.in);
+		sg = new StreamGobbler(new TestStream());
 		sg.start();
 	}
 
@@ -33,7 +46,7 @@ public class StreamGobblerTest{
 		sg = new StreamGobbler(null);
 		assertNotNull("StreamGobbler not null", sg);
 
-		sg = new StreamGobbler(System.in);
+		sg = new StreamGobbler(new TestStream());
 		assertNotNull("StreamGobbler not null", sg);
 	}
 
diff --git a/systemtap/org.eclipse.linuxtools.systemtap.ui.structures/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobbler.java b/systemtap/org.eclipse.linuxtools.systemtap.ui.structures/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobbler.java
index 3903a147a8..ec83f5e70c 100644
--- a/systemtap/org.eclipse.linuxtools.systemtap.ui.structures/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobbler.java
+++ b/systemtap/org.eclipse.linuxtools.systemtap.ui.structures/src/org/eclipse/linuxtools/systemtap/ui/structures/runnable/StreamGobbler.java
@@ -17,8 +17,6 @@ import java.util.ArrayList;
 
 import org.eclipse.linuxtools.systemtap.ui.structures.listeners.IGobblerListener;
 
-
-
 /**
  * A separate thread to listen to an InputStream and pull all the data
  * out of it. When data is found a new event is fired share the data with
@@ -26,13 +24,13 @@ import org.eclipse.linuxtools.systemtap.ui.structures.listeners.IGobblerListener
  * @author Ryan Morse
  */
 public class StreamGobbler implements Runnable {
+
 	public StreamGobbler(InputStream is) {
 		if(null != is) {
 			this.is = is;
 			line = new StringBuilder();
 			listeners = new ArrayList<IGobblerListener>();
 		}
-		locked = false;
 	}
 	
 	/**
@@ -43,65 +41,62 @@ public class StreamGobbler implements Runnable {
 	 */
 	//Make sure to call this method to start the StreamGobbler
 	public void start() {
-		t = new Thread(this, "StreamGobbler");
-		t.start();
+		reader = new Thread(this, "StreamGobbler"); //$NON-NLS-1$
+		reader.start();
 	}
-	
+
 	/**
 	 * Checks to see if the gobbler is still running.
 	 * @return boolean representing whether or not it is sill running
 	 */
 	public boolean isRunning() {
-		return (null != t);
+		return (null != reader);
 	}
 
 	/**
 	 * The main method of this class. It monitors the provided thread to see
 	 * when new data is available and then appends it to its current list of
 	 * data.  When a new line is read it will fire a DataEvent for listeners
-	 * to get ahold of the data.
+	 * to get a hold of the data.
 	 */
 	public void run() {
-		Thread thisThread = Thread.currentThread();
+		if (reader != Thread.currentThread())
+			return;
+
 		try {
-			int val=-1;
+			int val = is.read();
+			while(val != -1) {
+				line.append((char)val);
+
+				if ('\n' == val)
+					this.fireNewDataEvent();
 
-			while(t == thisThread) {
-				while(0 < is.available()) {
-					if(-1 == (val = is.read()))
-						this.stop();
-					else
-						line.append((char)val);
-					if ('\n' == val)
-						this.fireNewDataEvent();
-					
-				}
-				try {
-					Thread.sleep(10);
-				} catch(InterruptedException ie) {}
+				val = is.read();
 			}
-		} catch (IOException ioe) {}	//If stream closed before thread shuts down
+		} catch (IOException ioe) {}	// If stream closed before thread shuts down
 	}
 
 	/**
-	 * Stops the gobbler from monitering the stream, and fires one last data event
+	 * Stops the gobbler from monitoring the stream, and fires one last data event
 	 * to make sure that listeners have the entire contents of what was read in
 	 * from the stream.
 	 */
 	public synchronized void stop() {
-		try {	//Make sure we don't stop while there is still data in the stream
-			while(0 != is.available()) {
-				Thread.sleep(10);
-			}
-		} catch(Exception e) {}
-		t = null;
+		try {
+			// Interrupt the thread just in case it is blocked on a read.
+			reader.interrupt();
+			// Wait for the reader thread to finish.
+			reader.join();
+		} catch (InterruptedException e) {
+			// The thread was interrupted; nothing to do; finish stopping.
+		}
+		reader = null;
 		notify();
-		//Fire one last time to ensure listeners have gotten everything.
+		// Fire one last time to ensure listeners have gotten everything.
 		this.fireNewDataEvent();
 	}
 	
-	/**
-	 * Method for getting the most recently read line from the stream.
+	/**	 * Method for getting the most recently read line from the stream.
 	 * @return String representing the current line being read from the 
 	 * <code>InputStream</code>
 	 */
@@ -116,86 +111,61 @@ public class StreamGobbler implements Runnable {
 		if(isRunning())
 			stop();
 		line = null;
-		t = null;
+		reader = null;
 		is = null;
 	}
 	
 	/**
-	 * Fires new events to everything that is monitering this stream. Then clears
+	 * Fires new events to everything that is monitoring this stream. Then clears
 	 * the current line of data.
 	 */
 	private void fireNewDataEvent() {
-		//Implement our own lock since using synchronized causes a deadlock here
-	/*	while(locked) {
-			try {
-				wait(10);
-			} catch(Exception e) {}
-		}
-		locked = true;*/
-		for(int i = 0; i < listeners.size(); i++)
-		{
-		
-			listeners.get(i).handleDataEvent(line.toString());
-		}
+		this.fireNewDataEvent(line.toString());
 		line.delete(0, line.length());
-		locked = false;
 	}
 	
 	public void fireNewDataEvent(String l) {
-		//Implement our own lock since using synchronized causes a deadlock here
-	/*	while(locked) {
-			try {
-				wait(10);
-			} catch(Exception e) {}
-		}
-		locked = true;*/
-		for(int i = 0; i < listeners.size(); i++)
-		{
-			listeners.get(i).handleDataEvent(l);
+		synchronized (listeners) {
+			for(int i = 0; i < listeners.size(); i++){
+				listeners.get(i).handleDataEvent(l);
+			}
 		}
-//		line.delete(0, line.length());
-		locked = false;
 	}
 	
 	/**
 	 * Registers the provided listener to get data events.
-	 * @param l A listener that needs to moniter the stream.
+	 * @param l A listener that needs to monitor the stream.
 	 */
 	public void addDataListener(IGobblerListener l) {
-		
-		if(l != null && !listeners.contains(l))
-		{
-			
-			listeners.add(l);
-			
+		synchronized (listeners) {
+			if(l != null && !listeners.contains(l)){
+				listeners.add(l);
+			}
 		}
-		
 	}
 
 	/**
-	 * Unregisters the provied listener from getting new data events.
-	 * @param l A listener that is monitering the stream and should be removed
+	 * Unregisters the provided listener from getting new data events.
+	 * @param l A listener that is monitoring the stream and should be removed
 	 */
 	public void removeDataListener(IGobblerListener l) {
-		
-		if(listeners.contains(l))
-			listeners.remove(l);
+		synchronized (listeners) {
+			if(listeners.contains(l))
+				listeners.remove(l);
+		}
 	}
 
 	/**
 	 * Returns a list of all of the <code>IGobblerListeners</code> that
-	 * are lstening for data events.
+	 * are listening for data events.
 	 * @return ArrayList of all of the listeners registered.
 	 */
 	public ArrayList<IGobblerListener> getDataListeners() {
 		return listeners;
 	}
-	
+
 	private ArrayList<IGobblerListener> listeners;
 	private StringBuilder line;
-	private Thread t;
+	private Thread reader;
 	private InputStream is;
-	
-	@SuppressWarnings("unused")
-	private boolean locked;
 }
\ No newline at end of file