Changeset 31638 for main

Timestamp:

2017-05-01T18:23:39+12:00 (7 years ago)

Author:

ak19

Message:

Rewriting GShell.runLocal() to use SafeProcess instead of Process. This required overhaul of SafeProcess to correctly deal with interrupts as the build/import process launched in GShell can be cancelled from the CreatePane. SafeProcess now also keeps track of its internal process object and has static logging methods to simplify the changes required when swapping between the GS3 version and GS2 version of SafeProcess. May eventually call DebugStream methods from the log methods for GLI. Will add a DEBUG flag to allow verbose logging.

Location:

main/trunk/gli/src/org/greenstone/gatherer

Files:

• collection/CollectionManager.java (modified) (3 diffs)
• gui/CreatePane.java (modified) (4 diffs)
• shell/GShell.java (modified) (9 diffs)
• util/SafeProcess.java (modified) (41 diffs)

main/trunk/gli/src/org/greenstone/gatherer/collection/CollectionManager.java

@@ -246 +246 @@
     GShell shell = new GShell(command_parts, GShell.BUILD, BUILDING, this, build_monitor, GShell.GSHELL_BUILD);
     shell.addGShellListener(Gatherer.g_man.create_pane);
+    Gatherer.g_man.create_pane.setGShell(shell); // allow the create pane to call shell.cancel()
     shell.addGShellListener(Gatherer.g_man.format_pane);
     shell.start();
@@ -367 +368 @@
         GShell shell = new GShell(sched_parts, GShell.SCHEDULE, SCHEDULING, this, schedule_monitor, GShell.GSHELL_SCHEDULE);
     shell.addGShellListener(Gatherer.g_man.create_pane);
+    Gatherer.g_man.create_pane.setGShell(shell); // allow the create pane to call shell.cancel()
     shell.addGShellListener(Gatherer.g_man.format_pane);
     shell.start();
@@ -1227 +1229 @@
     //shell.setEventProperty("is_incremental", Boolean.toString(is_incremental));
     shell.addGShellListener(Gatherer.g_man.create_pane);
+    Gatherer.g_man.create_pane.setGShell(shell); // allow the create pane to call shell.cancel()
         shell.addGShellListener(Gatherer.g_man.format_pane);
     shell.start();

main/trunk/gli/src/org/greenstone/gatherer/gui/CreatePane.java

@@ -156 +156 @@
     public String homepage = null;
 
+    /** Access to the GShell object that this CreatePane listens to events for. 
+     * A handle to the GShell is needed order to interrupt any processes the GShell runs
+     * when the user cancels a build operation.
+    */
+    private GShell shell = null;
+
     /** The constructor creates important helper classes, and initializes all the components.
      * @see org.greenstone.gatherer.collection.CollectionManager
@@ -291 +297 @@
     }
 
+    public void setGShell(GShell shell) {
+    this.shell = shell;
+    }
 
     public void destroy() {
@@ -840 +849 @@
     private class CancelButtonListener
     implements ActionListener {
-    /** This method attempts to cancel the current GShell task. It does this by first telling CollectionManager not to carry out any further action. This it turn tells the GShell to break from the current job immediately, without waiting for the processEnded message, and then kills the thread in an attempt to stop the process. The results of such an action are debatable.
+    /** This method attempts to cancel the current GShell task. It does this by first telling CollectionManager not to carry out any further action. 
+     * Previously, this would in turn tell the GShell to break from the current job immediately, without waiting for the processEnded message, and then kills the thread in an attempt to stop the process. The results of such an action are debatable.
+     * Now, pressing cancel will send an interrupt to the GShell thread, which is the thread in which the external process is run (via the SafeProcess object). Interrupting a running SafeProcess will then interrupt any worker threads and destroy the process, with SafeProcess cleaning up after itself after its worker threads finished cleaning up after themselves. Tested on Linux.
      * @param event An <strong>ActionEvent</strong> who, thanks to the power of object oriented programming, we don't give two hoots about.
      * @see org.greenstone.gatherer.Gatherer
@@ -867 +878 @@
         build_monitor.setStop(true);
         build_monitor.reset();
+        // Tell the GShell to cleanly stop running its external process
+        if(CreatePane.this.shell != null) {
+        // We can call GShell.cancel() even if the GShell thread is blocking when running a process,
+        // because this CreatePane is running in its own separate GUI thread. This is because the
+        // process blocking call (process.waitFor()) and cancel() are not sequential operations.
+        CreatePane.this.shell.cancel();
+        }
 
         // Remove the collection lock.

main/trunk/gli/src/org/greenstone/gatherer/shell/GShell.java

@@ -56 +56 @@
 import org.greenstone.gatherer.metadata.DocXMLFileManager;
 import org.greenstone.gatherer.remote.RemoteGreenstoneServer;
-import org.greenstone.gatherer.util.StaticStrings; 
+import org.greenstone.gatherer.util.SafeProcess;
+import org.greenstone.gatherer.util.StaticStrings;
 import org.greenstone.gatherer.util.Utility;
 
@@ -63 +64 @@
  */
 public class GShell
-    extends Thread {
+    extends Thread implements SafeProcess.ExceptionHandler {
     /** A flag used to determine if this process has been asked to cancel. */
     private boolean cancel = false;
@@ -84 +85 @@
     private String commandOutput = null;
 
+    /** The process safely run in this GShell. Make sure to set to null when done with. */
+    SafeProcess prcs = null;
+
     /** Elements in process type enumeration. */
     static final public int BUILD = 0;
@@ -113 +117 @@
     static public String GSHELL_FEDORA_COLDELETE = "gshell_fedora_col_delete";
 
-    /** Determine if the given process is still executing. It does this by attempting to throw an exception - not the most efficient way, but the only one as far as I know
-     * @param process the Process to test
-     * @return true if it is still executing, false otherwise
-     */
-    static public boolean processRunning(Process process) {
-    boolean process_running = false;
-
-    try {
-        process.exitValue(); // This will throw an exception if the process hasn't ended yet.
-    }
-    catch(IllegalThreadStateException itse) {
-        process_running = true;
-    }
-    catch(Exception exception) {
-        DebugStream.printStackTrace(exception);
-    }
-    return process_running;
-    }
 
     /** Constructor gatherer all the data required to create a new process, and emit meaningfull messages.
@@ -236 +222 @@
 
 
-    private void runLocal(String[] args, BufferedOutputStream bos) 
+    // old_runLocal uses a potentially unsafe way of running a process and an inefficient way of reading
+    // from the process stdout and stderr streams. Replaced by new runLocal() which uses SafeProcess and
+    // the new CustomProcessHandler inner class instantiated for each of the 2 process output streams.
+    private void old_runLocal(String[] args, BufferedOutputStream bos) 
     {
     try {
@@ -260 +249 @@
         StringBuffer stdline_buffer = new StringBuffer();
     
-        while(/*type != GShell.NEW &&*/ processRunning(prcs) && !hasSignalledStop()) {
+        while(/*type != GShell.NEW &&*/ SafeProcess.processRunning(prcs) && !hasSignalledStop()) {
         // Hopefully this doesn't block if the process is trying to write to STDOUT.
         if((eisr!=null) && eisr.ready()) { 
@@ -348 +337 @@
     }
     
+    private void runLocal(String[] args, BufferedOutputStream bos) 
+    {
+    // in case we stop between import and build, let's not launch further processes
+    // that only pressing cancel, which results in interrupts, can subsequently stop.
+    if(hasSignalledStop()) {
+        return;
+    }
+
+    String command = "";
+    for(int i = 0; i < args.length; i++) {
+        command = command + args[i] + " ";
+    }
+    
+    ///ystem.err.println("Command: " + command);
+    fireMessage(type, Dictionary.get("GShell.Command") + ": " + command, status, null);
+    
+    prcs = new SafeProcess(args);
+    SafeProcess.CustomProcessHandler processOutHandler
+        = new SynchronizedProcessHandler(bos, SafeProcess.STDOUT);
+    SafeProcess.CustomProcessHandler processErrHandler
+        = new SynchronizedProcessHandler(bos, SafeProcess.STDERR);
+    
+    prcs.setExceptionHandler(this); 
+    int exitValue = prcs.runProcess(null, processOutHandler, processErrHandler); // use default procIn handling
+    
+    if(exitValue == 0) {
+        System.err.println("*** Exited normally");
+        status = OK;
+        fireMessage(type, typeAsString(type) + "> " + Dictionary.get("GShell.Success"), status, null);
+    }
+    else {
+        System.err.println("*** Exited abnormally with exit value " + exitValue);
+        status = ERROR;
+        fireMessage(type, typeAsString(type) + "> " + Dictionary.get("GShell.Failure"), status, null);
+    }
+    
+    prcs = null;
+    }
+
+    /** When cancel is called on this GShell thread from a separate thread, 
+     * This GShell will safely terminate any process it's currently running (by interrupting it)
+     * and will set this.prcs to null at the end. */
+    public void cancel() {
+    if(prcs != null) { //GShell is running a process, so interrupt the GShell/SafeProcess thread 
+
+        SafeProcess.log("********** HAS SIGNALLED STOP. INTERRUPTING THE GSHELL/SAFEPROCESS THREAD");
+
+        this.interrupt(); // interrupt this thread which is running the SafeProcess prcs
+        // this will propagate the CANCEL status
+        prcs = null;        
+    }
+    hasSignalledStop(); // synchronized. Will set status to CANCELLED in thread-safe manner.    
+    }
     
 
@@ -589 +631 @@
      * @return A <strong>boolean</strong> indicating if the user wanted to stop.
      */
-    public boolean hasSignalledStop() {
+    public synchronized boolean hasSignalledStop() {
     boolean has_signalled_stop = false;
     if(progress != null) {
@@ -639 +681 @@
     return name;
     }
+
+    // From interface SafeProcess.ExceptionHandler
+    // Called when an exception happens during the running of our perl process, as we want to set
+    // the GShell status to ERROR.
+    // However, exceptions when reading from our perl process' stderr and stdout streams are handled
+    // by SynchronizedProcessHandler.gotException() below, since they happen in separate threads
+    // from this one (the one from which the perl process is run).
+    public void gotException(Exception e) { 
+
+    if(e instanceof InterruptedException) {
+        SafeProcess.log("*** InterruptedException in GShell.runLocal()");
+        setStatus(CANCELLED); // expected exception
+    } else {
+        DebugStream.println("Exception in GShell.runLocal() - unexpected");
+        DebugStream.printStackTrace(e);
+        setStatus(ERROR); // status particularly needs to be synchronized on
+    }
+    }
+
+    public synchronized void setStatus(int newStatus) {
+    status = newStatus;
+    }
+
+    // Each instance of this class is run in its own thread by class SafeProcess.InputGobbler.
+    // This class deals with each incoming line from the perl process' stderr or stdout streams. One
+    // instance of this class for each stream. However, since multiple instances of this CustomProcessHandler
+    // could be (and in fact, are) writing to the same file in their own threads, several objects, not just
+    // the bufferedwriter object, needed to be made threadsafe.
+    // This class also handles exceptions during the running of the perl process.
+    // The runPerlCommand code originally would do a sendProcessStatus on each exception, so we ensure
+    // we do that here too, to continue original behaviour. These calls are also synchronized to make their
+    // use of the EventListeners threadsafe.
+    protected class SynchronizedProcessHandler extends SafeProcess.CustomProcessHandler
+    {
+    private final BufferedOutputStream bos; // needs to be final to be able to synchronize on the shared object
+    
+    public SynchronizedProcessHandler(BufferedOutputStream bos, int src) {
+        super(src); // will set this.source to STDERR or STDOUT
+        this.bos = bos; // caller will close bw, since many more than one
+                            // SynchronizedProcessHandlers are using it
+    }
+
+    // trying to keep synchronized methods as short as possible
+    private void logException(Exception e) {
+        String stream = (this.source == SafeProcess.STDERR) ? "stderr" : "stdout";
+        String msg = "Got exception when processing the perl process' " + stream + " stream.";
+
+        DebugStream.println(msg); // method is already synchronized
+        DebugStream.printStackTrace(e); // method is already synchronized
+        SafeProcess.log("*** " + msg, e);
+
+        GShell.this.setStatus(GShell.ERROR);
+    }
+
+    private void log(String msg) {
+        DebugStream.println(msg); // already synchro
+        System.err.println("@@@@@ " + msg);
+    }
+
+    public void run(Closeable inputStream) {
+        InputStream is = (InputStream) inputStream;
+
+        BufferedReader br = null;
+        try {       
+
+        br = new BufferedReader(new InputStreamReader(is, "UTF-8"));
+        String line=null;
+
+        ///while ( !GShell.this.prcs.processRunning() || hasSignalledStop()) { // need to sync on prcs
+        while ( !Thread.currentThread().isInterrupted() && (line = br.readLine()) != null ) {
+            this.gotLine(line); // synchronized         
+        }       
+
+        } catch (Exception e) {
+        logException(e);
+        } finally {
+        System.err.println("*********");
+        if(Thread.currentThread().isInterrupted()) {
+            log("We've been asked to stop.");
+            
+        }
+        SafeProcess.closeResource(br);
+        String stream = (this.source == SafeProcess.STDERR) ? "stderr" : "stdout";
+        System.err.println("*** In finally of " + stream + " stream");
+        System.err.println("*********");
+        }
+    }
+
+    protected synchronized void gotLine(String line) {
+        fireMessage(GShell.this.type,   // not final, needs synchro
+            GShell.this.typeAsString(type) + "> " + line,
+            GShell.this.status, // not final, needs synchro
+            this.bos);          // needs synchro
+    }
+
+    }
+
 }

main/trunk/gli/src/org/greenstone/gatherer/util/SafeProcess.java

@@ -10 +10 @@
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
-
 import java.util.Arrays;
 
 import org.apache.log4j.*;
+
+import org.greenstone.gatherer.DebugStream;
 
 // Use this class to run a Java Process. It follows the good and safe practices at
@@ -23 +24 @@
     public static final int STDERR = 0;
     public static final int STDOUT = 1;
-    public static final int STDIN = 2;    
+    public static final int STDIN = 2;
+
+    // charset for reading process stderr and stdout streams
+    //public static final String UTF8 = "UTF-8";    
 
     ///static Logger logger = Logger.getLogger(org.greenstone.util.SafeProcess.class.getName());
@@ -33 +37 @@
     private File dir = null;
     private String inputStr = null;
+    private Process process = null;
 
     // output from running SafeProcess.runProcess()
@@ -38 +43 @@
     private String errorStr = "";
     private int exitValue = -1;
+    //private String charset = null;
 
     // allow callers to process exceptions of the main process thread if they want
@@ -74 +80 @@
     public int getExitValue() { return exitValue; }
 
-    
+    //public void setStreamCharSet(String charset) { this.charset = charset; } 
+
     // set any string to send as input to the process spawned by SafeProcess
     public void setInputString(String sendStr) {
@@ -94 +101 @@
     }
 
+    // logger and DebugStream print commands are synchronized, therefore thread safe.
+    public static void log(String msg) {
+    //logger.info(msg);
+
+    System.err.println(msg);
+
+    //DebugStream.println(msg);
+    }
+
+    public static void log(String msg, Exception e) { // Print stack trace on the exception
+    //logger.error(msg, e);
+
+    System.err.println(msg);
+    e.printStackTrace();
+
+    //DebugStream.println(msg);
+    //DebugStream.printStackTrace(e);
+    }
+
+    public static void log(Exception e) {
+    //logger.error(e);
+
+    e.printStackTrace();
+
+    //DebugStream.printStackTrace(e);
+    }
+
+    public static void log(String msg, Exception e, boolean printStackTrace) {
+    if(printStackTrace) { 
+        log(msg, e);
+    } else {
+        log(msg);
+    }
+    }
+
     private Process doRuntimeExec() throws IOException {
     Process prcs = null;
@@ -99 +141 @@
     
     if(this.command != null) {
-        //logger.info("SafeProcess running: " + command);
-        System.err.println("SafeProcess running: " + command_args);
+        log("SafeProcess running: " + command);
         prcs = rt.exec(this.command);
     }
@@ -106 +147 @@
         
         // http://stackoverflow.com/questions/5283444/convert-array-of-strings-into-a-string-in-java
-        //logger.info("SafeProcess running: " + Arrays.toString(command_args));
-        System.err.println("SafeProcess running: " + Arrays.toString(command_args));
+        log("SafeProcess running: " + Arrays.toString(command_args));
         
         if(this.envp == null) { 
@@ -114 +154 @@
         
         if(this.dir == null) {
-            ///logger.info("\twith: " + Arrays.toString(this.envp));
-            ///System.err.println("\twith: " + Arrays.toString(this.envp));
+            //log("\twith: " + Arrays.toString(this.envp));
             prcs = rt.exec(this.command_args, this.envp);
         } else {
-            ///logger.info("\tfrom directory: " + this.dir);
-            ///logger.info("\twith: " + Arrays.toString(this.envp));
-            ///System.err.println("\tfrom directory: " + this.dir);
-            ///System.err.println("\twith: " + Arrays.toString(this.envp));
-            prcs = rt.exec(this.command_args, this.envp, this.dir);
+            //log("\tfrom directory: " + this.dir);
+            //log("\twith: " + Arrays.toString(this.envp));         
+            prcs = rt.exec(this.command_args, this.envp, this.dir);
         }
         }
@@ -131 +168 @@
 
     // Copied from gli's gui/FormatConversionDialog.java
-    private int waitForWithStreams(Process prcs,
-                   SafeProcess.OutputStreamGobbler inputGobbler,
+    private int waitForWithStreams(SafeProcess.OutputStreamGobbler inputGobbler,
                    SafeProcess.InputStreamGobbler outputGobbler,
                    SafeProcess.InputStreamGobbler errorGobbler)
@@ -138 +174 @@
     {
 
-            // kick off the stream gobblers
-            inputGobbler.start();
-            errorGobbler.start();
-            outputGobbler.start();
-
-            // any error???
-            this.exitValue = prcs.waitFor(); // can throw an InterruptedException if process did not terminate
-
-            ///logger.info("Process exitValue: " + exitValue);
-        ///System.err.println("Process exitValue: " + exitValue); 
-
+    
+    // kick off the stream gobblers
+    inputGobbler.start();
+    errorGobbler.start();
+    outputGobbler.start();
+    
+    // any error???
+    try {
+        this.exitValue = process.waitFor(); // can throw an InterruptedException if process did not terminate
+    } catch(InterruptedException ie) {
+        
+        log("*** Process interrupted (InterruptedException). Expected to be a Cancel operation.");
+            // don't print stacktrace: an interrupt here is not an error, it's expected to be a cancel action
+        
+        // propagate interrupts to worker threads here
+        // unless the interrupt emanated from any of them in any join(),
+        // which will be caught by caller's catch on InterruptedException.
+        // Only if the thread that SafeProcess runs in was interrupted
+        // should we propagate the interrupt to the worker threads.
+        // http://stackoverflow.com/questions/2126997/who-is-calling-the-java-thread-interrupt-method-if-im-not
+        // "I know that in JCiP it is mentioned that you should never interrupt threads you do not own"
+        // But SafeProcess owns the worker threads, so it has every right to interrupt them
+        // Also read http://stackoverflow.com/questions/13623445/future-cancel-method-is-not-working?noredirect=1&lq=1
+        
+        // http://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
+        // http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
+        // "Only code that implements a thread's interruption policy may swallow an interruption request. General-purpose task and library code should never swallow interruption requests."
+        // Does that mean that since this code implements this thread's interruption policy, it's ok
+        // to swallow the interrupt this time and not let it propagate by commenting out the next line?
+        //Thread.currentThread().interrupt(); // re-interrupt the thread
+        
+        inputGobbler.interrupt();
+        errorGobbler.interrupt();
+        outputGobbler.interrupt();
+        
+    } finally {     
+        
+        //log("Process exitValue: " + exitValue);
+        
         // From the comments of 
         // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
@@ -154 +218 @@
         // if there's no waiting for the threads, call join() on each Thread (StreamGobbler) object.
         // From Thread API: join() "Waits for this thread (the thread join() is invoked on) to die."
+        
+        // Wait for each of the threads to die, before attempting to destroy the process
+        // Any of these can throw InterruptedExceptions too
+        // and will be processed by the calling function's catch on InterruptedException.
+        // However, no one besides us will interrupting these threads I think...
+        // and we won't be throwing the InterruptedException from within the threads...
+        // So if any streamgobbler.join() call throws an InterruptedException, that would be unexpected
+
         outputGobbler.join(); 
         errorGobbler.join();
-        inputGobbler.join(); 
-        
-
-        // set the variables the code that created a SafeProcess object may want to inspect
+        inputGobbler.join();
+        
+        
+        // set the variables that the code which created a SafeProcess object may want to inspect
         this.outputStr = outputGobbler.getOutput();
         this.errorStr = errorGobbler.getOutput();
@@ -165 +237 @@
         // Since we didn't have an exception, process should have terminated now (waitFor blocks until then)
         // Set process to null so we don't forcibly terminate it below with process.destroy()
-        prcs = null;
-
-        return this.exitValue;
-    }
-
+        this.process = null;        
+    }
+
+    // Don't return from finally, it's considered an abrupt completion and exceptions are lost, see
+    // http://stackoverflow.com/questions/18205493/can-we-use-return-in-finally-block
+    return this.exitValue;
+    }
+
+
+    public synchronized boolean processRunning() {
+    if(process == null) return false;
+    return SafeProcess.processRunning(this.process);
+    }
 
     // Run a very basic process: with no reading from or writing to the Process' iostreams,
     // this just execs the process and waits for it to return
     public int runBasicProcess() {
-    Process prcs = null;
     try {
         // 1. create the process
-        prcs = doRuntimeExec();
+        process = doRuntimeExec();
         // 2. basic waitFor the process to finish
-        this.exitValue = prcs.waitFor();
+        this.exitValue = process.waitFor();
 
         
@@ -186 +265 @@
         exceptionHandler.gotException(ioe);
         } else {
-        //logger.error("IOException: " + ioe.getMessage(), ioe);
-        System.err.println("IOException " + ioe.getMessage());
-        ioe.printStackTrace();
+        log("IOException: " + ioe.getMessage(), ioe);
         }
     } catch(InterruptedException ie) {
@@ -194 +271 @@
         if(exceptionHandler != null) {
         exceptionHandler.gotException(ie);
-        } else {
-        //logger.error("Process InterruptedException: " + ie.getMessage(), ie);
-        System.err.println("Process InterruptedException " + ie.getMessage());
-        ///ie.printStackTrace(); // an interrupt here is not an error, it can be a cancel action
+        } else { // Unexpected InterruptedException, so printstacktrace
+        log("Process InterruptedException: " + ie.getMessage(), ie);
         }
         
@@ -203 +278 @@
     } finally { 
 
-        if( prcs != null ) {
-        prcs.destroy(); // see runProcess() below
+        if( process != null ) {
+        process.destroy(); // see runProcess() below
         }
     }
@@ -222 +297 @@
                CustomProcessHandler procErrHandler)
     {
-    Process prcs = null;
     SafeProcess.OutputStreamGobbler inputGobbler = null;
     SafeProcess.InputStreamGobbler errorGobbler = null;
@@ -229 +303 @@
     try {
         // 1. get the Process object
-        prcs = doRuntimeExec();
+        process = doRuntimeExec();
         
 
@@ -238 +312 @@
         // send inputStr to process. The following constructor can handle inputStr being null
         inputGobbler = // WriterToProcessInputStream
-            new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), this.inputStr);
+            new SafeProcess.OutputStreamGobbler(process.getOutputStream(), this.inputStr);
         } else { // user will do custom handling of process' InputStream 
-        inputGobbler = new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), procInHandler);
+        inputGobbler = new SafeProcess.OutputStreamGobbler(process.getOutputStream(), procInHandler);
         }
 
@@ -246 +320 @@
         if(procErrHandler == null) {
         errorGobbler // ReaderFromProcessOutputStream
-            = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), splitStdErrorNewLines);
+            = new SafeProcess.InputStreamGobbler(process.getErrorStream(), splitStdErrorNewLines);
         } else {
         errorGobbler
-            = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), procErrHandler);
+            = new SafeProcess.InputStreamGobbler(process.getErrorStream(), procErrHandler);
         }
 
@@ -255 +329 @@
         if(procOutHandler == null) {
         outputGobbler
-            = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), splitStdOutputNewLines);
+            = new SafeProcess.InputStreamGobbler(process.getInputStream(), splitStdOutputNewLines);
         } else {
         outputGobbler
-            = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), procOutHandler);
+            = new SafeProcess.InputStreamGobbler(process.getInputStream(), procOutHandler);
         }
 
 
             // 3. kick off the stream gobblers
-        this.exitValue = waitForWithStreams(prcs, inputGobbler, outputGobbler, errorGobbler);
+        this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
        
     } catch(IOException ioe) {
@@ -269 +343 @@
         exceptionHandler.gotException(ioe);
         } else {
-        //logger.error("IOexception: " + ioe.getMessage(), ioe);
-        System.err.println("IOexception " + ioe.getMessage());
-        ioe.printStackTrace();
-        }
-    } catch(InterruptedException ie) {
-
+        log("IOexception: " + ioe.getMessage(), ioe);
+        }
+    } catch(InterruptedException ie) { // caused during any of the gobblers.join() calls, this is unexpected so print stack trace
+        
         if(exceptionHandler != null) {
         exceptionHandler.gotException(ie);
+        log("@@@@ Unexpected InterruptedException when waiting for process stream gobblers to die");
         } else {
-        //logger.error("Process InterruptedException: " + ie.getMessage(), ie);
-        System.err.println("Process InterruptedException " + ie.getMessage());
-        ///ie.printStackTrace(); // an interrupt here is not an error, it can be a cancel action
-        }
-
-        // propagate interrupts to worker threads here?
-        // unless the interrupt emanated from any of them in any join()...
-        // Only if the thread SafeProcess runs in was interrupted
-        // do we propagate the interrupt to the worker threads.
-        // http://stackoverflow.com/questions/2126997/who-is-calling-the-java-thread-interrupt-method-if-im-not
-        // "I know that in JCiP it is mentioned that you should never interrupt threads you do not own"
-        // But SafeProcess owns the worker threads, so it have every right to interrupt them
-        // Also read http://stackoverflow.com/questions/13623445/future-cancel-method-is-not-working?noredirect=1&lq=1
-        if(Thread.currentThread().isInterrupted()) {
-        inputGobbler.interrupt();
-        errorGobbler.interrupt();
-        outputGobbler.interrupt();      
-        }
-
-        // On catchingInterruptedException, re-interrupt the thread.
-        // This is just how InterruptedExceptions tend to be handled
-        // See also http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
-        // and https://praveer09.github.io/technology/2015/12/06/understanding-thread-interruption-in-java/
-
-        // http://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
-        // http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
-        // "Only code that implements a thread's interruption policy may swallow an interruption request. General-purpose task and library code should never swallow interruption requests."
-        // Does that mean that since this code implements this thread's interruption policy, it's ok
-        // to swallow the interrupt this time and not let it propagate by commenting out the next line?
-        Thread.currentThread().interrupt(); // re-interrupt the thread - which thread? Infinite loop?
+        log("*** Unexpected InterruptException when waiting for process stream gobblers to die:" + ie.getMessage(), ie);
+        }
+
+        // see comments in other runProcess()
+        Thread.currentThread().interrupt();
+    
     } finally { 
 
-        // Moved into here from GS2PerlConstructor which said
-        // "I need to somehow kill the child process. Unfortunately Thread.stop() and Process.destroy() both fail to do this. But now, thankx to the magic of Michaels 'close the stream suggestion', it works fine."
-        // http://steveliles.github.io/invoking_processes_from_java.html
-        // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
-        // http://mark.koli.ch/leaky-pipes-remember-to-close-your-streams-when-using-javas-runtimegetruntimeexec        
-        if( prcs != null ) {        
-        prcs.destroy();
-        }
+        log("*** In finally of SafeProcess.runProcess(3 params)");
+
+        if( process != null ) {
+        log("*** Going to call process.destroy 2");
+        process.destroy();
+        process = null;
+        log("*** Have called process.destroy 2");
+        }
+        
     }
     
@@ -325 +375 @@
     public int runProcess(LineByLineHandler outLineByLineHandler, LineByLineHandler errLineByLineHandler)
     {
-    Process prcs = null;
     SafeProcess.OutputStreamGobbler inputGobbler = null;
     SafeProcess.InputStreamGobbler errorGobbler = null;
@@ -332 +381 @@
     try {
         // 1. get the Process object
-        prcs = doRuntimeExec();
+        process = doRuntimeExec();
         
 
@@ -340 +389 @@
         // send inputStr to process. The following constructor can handle inputStr being null
         inputGobbler = // WriterToProcessInputStream
-        new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), this.inputStr);
+        new SafeProcess.OutputStreamGobbler(process.getOutputStream(), this.inputStr);
 
         // PROC ERR STREAM to monitor for any error messages or expected output in the process' stderr    
         errorGobbler // ReaderFromProcessOutputStream
-            = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), splitStdErrorNewLines);     
+            = new SafeProcess.InputStreamGobbler(process.getErrorStream(), splitStdErrorNewLines);      
             // PROC OUT STREAM to monitor for the expected std output line(s)
         outputGobbler
-        = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), splitStdOutputNewLines);
+        = new SafeProcess.InputStreamGobbler(process.getInputStream(), splitStdOutputNewLines);
 
 
@@ -360 +409 @@
 
             // 4. kick off the stream gobblers
-        this.exitValue = waitForWithStreams(prcs, inputGobbler, outputGobbler, errorGobbler);
+        this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
        
     } catch(IOException ioe) {
@@ -366 +415 @@
         exceptionHandler.gotException(ioe);
         } else {
-        //logger.error("IOexception: " + ioe.getMessage(), ioe);
-        System.err.println("IOexception " + ioe.getMessage());
-        ioe.printStackTrace();
-        }
-    } catch(InterruptedException ie) {
-
+        log("IOexception: " + ioe.getMessage(), ioe);       
+        }
+    } catch(InterruptedException ie) { // caused during any of the gobblers.join() calls, this is unexpected so log it
+        
         if(exceptionHandler != null) {
         exceptionHandler.gotException(ie);
+        log("@@@@ Unexpected InterruptedException when waiting for process stream gobblers to die");
         } else {
-        //logger.error("Process InterruptedException: " + ie.getMessage(), ie);
-        System.err.println("Process InterruptedException " + ie.getMessage());
-        ///ie.printStackTrace(); // an interrupt here is not an error, it can be a cancel action
-        }
-
-        // propagate interrupts to worker threads here?
-        // unless the interrupt emanated from any of them in any join()...
-        // Only if the thread SafeProcess runs in was interrupted
-        // do we propagate the interrupt to the worker threads.
-        // http://stackoverflow.com/questions/2126997/who-is-calling-the-java-thread-interrupt-method-if-im-not
-        // "I know that in JCiP it is mentioned that you should never interrupt threads you do not own"
-        // But SafeProcess owns the worker threads, so it have every right to interrupt them
-        // Also read http://stackoverflow.com/questions/13623445/future-cancel-method-is-not-working?noredirect=1&lq=1
-        if(Thread.currentThread().isInterrupted()) {
-        inputGobbler.interrupt();
-        errorGobbler.interrupt();
-        outputGobbler.interrupt();      
-        }
-
-        // On catchingInterruptedException, re-interrupt the thread.
+        log("*** Unexpected InterruptException when waiting for process stream gobblers to die: " + ie.getMessage(), ie);
+        }
+        // We're not causing any interruptions that may occur when trying to stop the worker threads
+        // So resort to default behaviour in this catch?
+        // "On catching InterruptedException, re-interrupt the thread."
         // This is just how InterruptedExceptions tend to be handled
         // See also http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
         // and https://praveer09.github.io/technology/2015/12/06/understanding-thread-interruption-in-java/
-
-        // http://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
-        // http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
-        // "Only code that implements a thread's interruption policy may swallow an interruption request. General-purpose task and library code should never swallow interruption requests."
-        // Does that mean that since this code implements this thread's interruption policy, it's ok
-        // to swallow the interrupt this time and not let it propagate by commenting out the next line?
         Thread.currentThread().interrupt(); // re-interrupt the thread - which thread? Infinite loop?
+    
     } finally { 
 
-        // Moved into here from GS2PerlConstructor which said
-        // "I need to somehow kill the child process. Unfortunately Thread.stop() and Process.destroy() both fail to do this. But now, thankx to the magic of Michaels 'close the stream suggestion', it works fine."
+        // Moved into here from GS2PerlConstructor and GShell.runLocal() which said
+        // "I need to somehow kill the child process. Unfortunately Thread.stop() and Process.destroy() both fail to do this. But now, thankx to the magic of Michaels 'close the stream suggestion', it works fine (no it doesn't!)"
         // http://steveliles.github.io/invoking_processes_from_java.html
         // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
         // http://mark.koli.ch/leaky-pipes-remember-to-close-your-streams-when-using-javas-runtimegetruntimeexec        
-        if( prcs != null ) {        
-        prcs.destroy();
-        }
+        if( process != null ) {
+        log("*** Going to call process.destroy 1");
+        process.destroy();
+        process = null;
+        log("*** Have called process.destroy 1");
+        }
+        log("*** In finally of SafeProcess.runProcess(2 params)");
     }
     
@@ -452 +484 @@
     protected CustomProcessHandler(int src) {
     this.source = src; // STDERR or STDOUT or STDIN
+
+    // modify threadname to prefix stream src (stdin/stderr/stdout)
+    // Useful for debugging if thread is named clearly
+    String stream;
+    switch(src) {
+    case SafeProcess.STDERR:
+       stream = "stderr"; 
+    case SafeProcess.STDOUT:
+        stream = "stdout";
+    default:
+        stream = "stdin";
+    }
+    Thread.currentThread().setName(stream + Thread.currentThread().getName());
     }
     
@@ -487 +532 @@
     public InputStreamGobbler(InputStream is)
     {
+    super("InputStreamGobbler"); // thread name
     this.is = is;
     this.split_newlines = false;
@@ -493 +539 @@
     public InputStreamGobbler(InputStream is, boolean split_newlines)
     {
+    super("InputStreamGobbler"); // thread name
     this.is = is;
     this.split_newlines = split_newlines;
@@ -499 +546 @@
     public InputStreamGobbler(InputStream is, CustomProcessHandler customHandler)
     {
+    super("InputStreamGobbler"); // thread name
     this.is = is;
     this.customHandler = customHandler;
@@ -514 +562 @@
         br = new BufferedReader(new InputStreamReader(is, "UTF-8"));
         String line=null;
-        while ( (line = br.readLine()) != null ) {
-
-        if(this.isInterrupted()) { // should we not instead check if SafeProcess thread was interrupted?
-            //logger.info("Got interrupted when reading lines from process err/out stream.");
-            //System.err.println("InputStreamGobbler.runDefault() Got interrupted when reading lines from process err/out stream.");
-            break; // will go to finally block
-        }
-
-        //System.out.println("@@@ GOT LINE: " + line);
+        while ( !this.isInterrupted() && (line = br.readLine()) != null ) {
+
+        //log("@@@ GOT LINE: " + line);
         outputstr.append(line);
         if(split_newlines) {
@@ -532 +574 @@
         }
         }
+
     } catch (IOException ioe) {
         if(lineByLineHandler != null) {
         lineByLineHandler.gotException(ioe);
         } else {
-        //logger.error("Exception when reading from a process' stdout/stderr stream: ", ioe);
-        System.err.println("Exception when reading from a process' stdout/stderr stream: ");
-        ioe.printStackTrace();  
+        log("Exception when reading from a process' stdout/stderr stream: ", ioe);
         }
     } finally {
+        log("*********");
+        if(this.isInterrupted()) {
+        log("We've been asked to stop.");       
+        }
         SafeProcess.closeResource(br);
+        log("*** In finally of " + this.getName());
+        log("*********");
     }
     }
@@ -573 +620 @@
 
     public OutputStreamGobbler(OutputStream os) {
+    super("OutputStreamGobbler"); // thread name
     this.os = os;
     }
@@ -578 +626 @@
     public OutputStreamGobbler(OutputStream os, String inputstr)
     {
+    super("OutputStreamGobbler"); // thread name
     this.os = os;
     this.inputstr = inputstr;
@@ -583 +632 @@
 
     public OutputStreamGobbler(OutputStream os, CustomProcessHandler customHandler) {
+    super("OutputStreamGobbler"); // thread name
     this.os = os;
     this.customHandler = customHandler;
@@ -592 +642 @@
     if (inputstr == null) {
         return;
+    }
+
+    // also quit if the process was interrupted before we could send anything to its stdin
+    if(this.isInterrupted()) {
+        log(this.getName() + " thread was interrupted.");
+        return;
     }
     
@@ -615 +671 @@
         */
     } catch (IOException ioe) {
-        //logger.error("Exception writing to SafeProcess' inputstream: ", ioe);
-        System.err.println("Exception writing to SafeProcess' inputstream: ");
-        ioe.printStackTrace();  
+        log("Exception writing to SafeProcess' inputstream: ", ioe);
     } finally {
         SafeProcess.closeResource(osw);
@@ -635 +689 @@
         runCustom();
     }
-
     }   
 } // end static inner class OutputStreamGobbler
@@ -654 +707 @@
         }
     } catch(Exception e) {
-        //logger.error("Exception closing resource: ", e);
-        System.err.println("Exception closing resource: " + e.getMessage());
-        e.printStackTrace();
+        log("Exception closing resource: " + e.getMessage(), e);
         resourceHandle = null;
         success = false;
@@ -675 +726 @@
     }
 
+// Moved from GShell.java
+    /** Determine if the given process is still executing. It does this by attempting to throw an exception - not the most efficient way, but the only one as far as I know
+     * @param process the Process to test
+     * @return true if it is still executing, false otherwise
+     */
+    static public boolean processRunning(Process process) {
+    boolean process_running = false;
+
+    try {
+        process.exitValue(); // This will throw an exception if the process hasn't ended yet.
+    }
+    catch(IllegalThreadStateException itse) {
+        process_running = true;
+    }
+    catch(Exception exception) {
+        log(exception); // DebugStream.printStackTrace(exception);
+    }
+    return process_running;
+    }
+
 } // end class SafeProcess