Changeset 31574 for main/trunk/greenstone3

Timestamp:

2017-04-05T20:22:15+12:00 (7 years ago)

Author:

ak19

Message:

Trying to correct the GS2PerlConstructor.runPerlCommand() function which has always been brittle, and causing a deadlocked Process when things go wrong. The current solution is with the new class SafeProcess, which tries to properly create, run and end a process and manage its streams, and close them off. Have tried to ensure there will be no concurrency issues. Maybe it would be better to use Executor to instantiate threads (and to use its cancel abilities if we want this in future), but have stuck to existing code that worked. Changed GS2PerlConstructor to use this new SafeProcess class in its new runPerlCommand() method. Tested document editing and adding user comments on Linux to find these things still work. However, the deadlock was happening on windows, so that's where the solution should be tested. Leaving in debug statements for windows testing.

Location:

main/trunk/greenstone3/src/java/org/greenstone

Files:

• gsdl3/build/GS2PerlConstructor.java (modified) (7 diffs)
• util/SafeProcess.java (modified) (13 diffs)

main/trunk/greenstone3/src/java/org/greenstone/gsdl3/build/GS2PerlConstructor.java

@@ -5 +5 @@
 import org.greenstone.util.Misc;
 import org.greenstone.util.GlobalProperties;
+import org.greenstone.util.SafeProcess;
 
 // xml classes
@@ -27 +28 @@
  * perl scripts to do the building stuff
  */
-public class GS2PerlConstructor extends CollectionConstructor
+public class GS2PerlConstructor extends CollectionConstructor implements SafeProcess.ExceptionHandler
 {
     static Logger logger = Logger.getLogger(org.greenstone.gsdl3.build.GS2PerlConstructor.class.getName());
@@ -374 +375 @@
     return runPerlCommand(command, null, null);
     }
-    
-    protected boolean runPerlCommand(String[] command, String[] envvars, File dir)
+
+        
+    protected boolean runPerlCommand(String[] command, String[] envvars, File dir)
+    {
+    boolean success = true;
+    
+    int sepIndex = this.gsdl3home.lastIndexOf(File.separator);
+    String srcHome = this.gsdl3home.substring(0, sepIndex);
+    
+    ArrayList<String> args = new ArrayList<String>();
+    args.add("GSDLHOME=" + this.gsdl2home);
+    args.add("GSDL3HOME=" + this.gsdl3home);
+    args.add("GSDL3SRCHOME=" + srcHome);
+    args.add("GSDLOS=" + this.gsdlos);
+    args.add("GSDL-RUN-SETUP=true");
+    args.add("PERL_PERTURB_KEYS=0");
+    
+    if(envvars != null) {
+        for(int i = 0; i < envvars.length; i++) {
+        args.add(envvars[i]);
+        }
+    }
+    
+    for (String a : System.getenv().keySet()) {
+        args.add(a + "=" + System.getenv(a));
+    }
+    
+    String command_str = "";
+    for (int i = 0; i < command.length; i++) {
+        command_str = command_str + command[i] + " ";
+    }
+    
+    sendMessage(new ConstructionEvent(this, GSStatus.INFO, "command = " + command_str));
+    
+    
+    logger.info("### Running command = " + command_str);
+
+    // This is where we create and run our perl process safely
+    SafeProcess perlProcess 
+        = new SafeProcess(command, args.toArray(new String[args.size()]), dir); //  dir can be null
+    
+    
+    sendProcessBegun(new ConstructionEvent(this, GSStatus.ACCEPTED, "starting"));
+    
+    File logDir = new File(GSFile.collectDir(this.site_home) + File.separator + this.collection_name + File.separator + "log");
+    if (!logDir.exists()) {
+        logDir.mkdir();
+    }
+    
+    // Only from Java 7+: Try-with-Resources block will safely close the BufferedWriter
+    // https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
+    BufferedWriter bw = null;
+    try {
+        bw = new BufferedWriter(new FileWriter(GSFile.collectDir(this.site_home) + File.separator + this.collection_name + File.separator + "log" + File.separator + "build_log." + (System.currentTimeMillis()) + ".txt"));        
+    
+        bw.write("Document Editor Build \n");       
+        bw.write("Command = " + command_str + "\n");
+        
+        // handle each incoming line from stdout and stderr streams, and any exceptions that occur then
+        SynchronizedProcessLineByLineHandler outLineByLineHandler
+        = new SynchronizedProcessLineByLineHandler(bw, SynchronizedProcessLineByLineHandler.STDOUT);
+        SynchronizedProcessLineByLineHandler errLineByLineHandler
+        = new SynchronizedProcessLineByLineHandler(bw, SynchronizedProcessLineByLineHandler.STDERR);
+        perlProcess.setStdOutLineByLineHandler(outLineByLineHandler);
+        perlProcess.setStdErrLineByLineHandler(errLineByLineHandler);
+        
+        // GS2PerlConstructor will do further handling of exceptions that may occur during the perl
+        // process (including if writing something to the process' inputstream, not that we're doing that for this perlProcess)
+        perlProcess.setExceptionHandler(this); 
+        
+        // finally, execute the process
+        
+        // Captures the std err of a program and pipes it into
+        // std in of java, as before.
+
+        logger.info("**** BEFORE runProcess.");
+        perlProcess.runProcess();
+        logger.info("**** AFTER runProcess:");
+        
+    // The original runPerlCommand() code had an ineffective check for whether the cmd had been cancelled
+    // midway through executing the perl, as condition of a while loop reading from stderr and stdout.
+    // We don't include the cancel check here, as superclass CollectionConstructor.stopAction(), which set
+    // this.cancel to true, never got called anywhere.
+    // But I think a proper cancel of our perl process launched by this GS2PerlConstructor Thread object
+    // and of the worker threads it launches, could be implemented with interrupts. See:
+    // http://stackoverflow.com/questions/6859681/better-way-to-signal-other-thread-to-stop
+    // https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html
+    // https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#interrupted()
+    // https://praveer09.github.io/technology/2015/12/06/understanding-thread-interruption-in-java/
+    // The code that calls GS2PerlConstructor.stopAction() should also call GSPerlConstructor.interrupt()
+    // Then in SafeProcess.runProcess(), I think the waitFor() will throw an InterruptedException()
+    // This can be caught and interrupt() called on SafeProcess' workerthreads, 
+    // Any workerthreads' run() methods that block (IO, loops) can test this.isInterrupted() 
+    // and can break out of any loops and release resources in finally.
+    // Back in SafeProcess.runProcess, the InterruptedException catch block will be followed by finally
+    // that will clear up any further resources and destroy the process forcibly if it hadn't been ended.
+
+    } catch(IOException e) {
+        e.printStackTrace();
+        sendProcessStatus(new ConstructionEvent(this, GSStatus.ERROR, "Exception occurred " + e.toString()));
+    } finally {
+        SafeProcess.closeResource(bw);
+    }
+
+    if (!this.cancel) {
+        // Now display final message based on exit value            
+        
+        if (perlProcess.getExitValue() == 0) {  
+        //status = OK;
+        sendProcessStatus(new ConstructionEvent(this, GSStatus.CONTINUING, "Success"));
+        
+        success = true;
+        } else {
+        //status = ERROR;
+        sendProcessStatus(new ConstructionEvent(this, GSStatus.ERROR, "Failure"));
+        
+        //return false;
+        success = false;
+        
+        }
+    } else { // cancelled
+        
+        // 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.
+        sendProcessStatus(new ConstructionEvent(this, GSStatus.HALTED, "killing the process"));
+        //prcs.getOutputStream().close();
+        //prcs.destroy();
+        ////status = ERROR;
+        
+        //return false;
+        success = false;
+    }
+    // we're done, but we don't send a process complete message here cos there might be stuff to do after this has finished.
+    //return true;      
+    return success;
+    }
+    
+
+    // this method is blocking again, on Windows when adding user comments
+    protected boolean old_runPerlCommand(String[] command, String[] envvars, File dir)
     {
         boolean success = true;
@@ -406 +544 @@
             command_str = command_str + command[i] + " ";
         }
+
+logger.info("### old runPerlCmd, command = " + command_str);
 
         sendMessage(new ConstructionEvent(this, GSStatus.INFO, "command = " + command_str));
@@ -454 +594 @@
                 }
             }
-            closeResource(bw); 
+            SafeProcess.closeResource(bw); 
             
             if (!this.cancel)
@@ -500 +640 @@
         
              if( prcs != null ) {
-                closeResource(prcs.getErrorStream());
-                closeResource(prcs.getOutputStream());
-                closeResource(prcs.getInputStream());
+                SafeProcess.closeResource(prcs.getErrorStream());
+                SafeProcess.closeResource(prcs.getOutputStream());
+                SafeProcess.closeResource(prcs.getInputStream());
                 prcs.destroy();
             }
         
-            closeResource(ebr);
-            closeResource(stdinbr);
+            SafeProcess.closeResource(ebr);
+            SafeProcess.closeResource(stdinbr);
         }
 
@@ -514 +654 @@
         return success;
     }
-    
-    public static void closeResource(Closeable resourceHandle) {
-        try {
-            if(resourceHandle != null) {
-                resourceHandle.close();
-                resourceHandle = null;
-            }
-        } catch(Exception e) {
-            System.err.println("Exception closing resource: " + e.getMessage());
-            e.printStackTrace();
-        }
+
+
+    // From interface SafeProcess.ExceptionHandler
+    // Called when an exception happens during the running of our perl process. However,
+    // exceptions when reading from our perl process' stderr and stdout streams are handled by
+    // SynchronizedProcessLineByLineHandler.gotException() below.
+    public void gotException(Exception e) {
+
+    // do what original runPerlCommand() code always did when an exception occurred
+    // when running the perl process:
+    e.printStackTrace();
+    sendProcessStatus(new ConstructionEvent(this,GSStatus.ERROR, 
+                        "Exception occurred " + e.toString())); // atomic
     }
+    
+    // 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 LineByLineHandler 
+    // could be (and in fact, are) writing to the same file in their own threads, the writer object needs 
+    // 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.
+    protected class SynchronizedProcessLineByLineHandler implements SafeProcess.LineByLineHandler
+    {
+    public static final int STDERR = 0;
+    public static final int STDOUT = 1;
+
+    private final int source;
+    private final BufferedWriter bwHandle; // needs to be final to synchronize on the object
+        
+
+    public SynchronizedProcessLineByLineHandler(BufferedWriter bw, int src) {
+        this.bwHandle = bw;
+        this.source = src; // STDERR or STDOUT
+    }
+
+    public void gotLine(String line) {
+        //if(this.source == STDERR) {
+        ///System.err.println("ERROR: " + line);
+        //} else {
+        ///System.err.println("OUT: " + line);
+        //}
+
+        // BufferedWriter writes may not be atomic
+        // http://stackoverflow.com/questions/9512433/is-writer-an-atomic-method
+        // Choosing to put try-catch outside of sync block, since it's okay to give up lock on exception
+        // http://stackoverflow.com/questions/14944551/it-is-better-to-have-a-synchronized-block-inside-a-try-block-or-a-try-block-insi
+        // "All methods on Logger are multi-thread safe", see
+        // http://stackoverflow.com/questions/14211629/java-util-logger-write-synchronization
+
+        try {
+        synchronized(bwHandle) { // get a lock on the writer handle, then write
+        
+            bwHandle.write(line + "\n");
+        } 
+        } catch(IOException ioe) {
+        String msg = (source == STDERR) ? "stderr" : "stdout";
+        msg = "Exception when writing out a line read from perl process' " + msg + " stream.";
+        GS2PerlConstructor.logger.error(msg, ioe);
+        }
+        
+        // this next method is thread safe since only synchronized methods are invoked.
+        // and only immutable (final) vars are used.
+        sendProcessStatus(new ConstructionEvent(GS2PerlConstructor.this, GSStatus.CONTINUING, line));
+    }   
+
+    // This is called when we get an exception during the processing of a perl's
+    // input-, err- or output stream
+    public void gotException(Exception e) {
+        String msg = (source == STDERR) ? "stderr" : "stdout";
+        msg = "Got exception when processing the perl process' " + msg + " stream.";
+
+        // now do what the original runPerlCommand() code always did:
+        e.printStackTrace();
+        sendProcessStatus(new ConstructionEvent(this, GSStatus.ERROR, "Exception occurred " + e.toString())); // atomic
+    }
+
+    } // end inner class SynchronizedProcessLineByLineHandler
+
 }

main/trunk/greenstone3/src/java/org/greenstone/util/SafeProcess.java

@@ -23 +23 @@
     static Logger logger = Logger.getLogger(org.greenstone.util.SafeProcess.class.getName());
 
-    private String[] command_args;
+    private String[] command_args = null;
+    private String[] envp = null;
+    private File dir = null;
     private String inputStr = null;
 
@@ -29 +31 @@
     private String errorStr = "";
 
-    private int process_exitValue = -1;
-    private ProcessLineByLineHandler lineByLineHandler = null;
+    private int exitValue = -1;
+
+    // user can write custom LineByLineHandler to deal with stdout lines as they come out one line at a time
+    // and stderr lines as they come out one at a time
+    private LineByLineHandler errLineByLineHandler = null;
+    private LineByLineHandler outLineByLineHandler = null;
+    private ExceptionHandler exceptionHandler = null;
 
     // whether std/err output should be split at new lines
@@ -37 +44 @@
 
     // call one of these constructors
+
+    // cmd args version
     public SafeProcess(String[] cmd_args)
     {
-    this(cmd_args, null, null, false, false);
-    }
-
-    public SafeProcess(String[] cmd_args, ProcessLineByLineHandler lbl_handler)
-    {
-    this(cmd_args, lbl_handler, null, false, false);
-    }
-
-    public SafeProcess(String[] cmd_args, ProcessLineByLineHandler lbl_handler,
-               String sendStr, boolean splitStdOut, boolean splitStdErr)
-    {   
     command_args = cmd_args;
-    lineByLineHandler = lbl_handler;
-    setInputString(sendStr);
-    setSplitStdOutputNewLines(splitStdOut);
-    setSplitStdErrorNewLines(splitStdErr);
-    runProcess();
+    }
+
+    // cmd args with env version, launchDir can be null.
+    public SafeProcess(String[] cmd_args, String[] envparams, File launchDir)
+    {
+    command_args = cmd_args;
+    envp = envparams;
+    dir = launchDir;
     }
 
@@ -62 +63 @@
     public String getStdOutput() { return outputStr; }
     public String getStdError() { return errorStr; }
-    public int getExitValue() { return process_exitValue; }
-
+    public int getExitValue() { return exitValue; }
+
+    
+    // set any string to send as input to the process spawned by SafeProcess
     public void setInputString(String sendStr) {
     inputStr = sendStr;
     }
+
+    // register a handler whose gotLine() method will get called as each line is read from the process' stdout
+    public void setStdOutLineByLineHandler(LineByLineHandler out_lbl_handler) {
+    outLineByLineHandler = out_lbl_handler;
+    }
+
+    // register a handler whose gotLine() method will get called as each line is read from the process' stderr
+    public void setStdErrLineByLineHandler(LineByLineHandler err_lbl_handler) {
+    errLineByLineHandler = err_lbl_handler;
+    }
+
+    // register a SafeProcess ExceptionHandler whose gotException() method will
+    // get called for each exception encountered
+    public void setExceptionHandler(ExceptionHandler exception_handler) {
+    exceptionHandler = exception_handler;   
+    }
+
+    // set if you want the std output or err output to have \n at each newline read from the stream
     public void setSplitStdOutputNewLines(boolean split) {
     splitStdOutputNewLines = split;
@@ -75 +96 @@
 
 //***************** Copied from gli's gui/FormatConversionDialog.java *************//
-    private void runProcess() {
+    public void runProcess() {
+    Process prcs = null;
+    SafeProcess.OutputStreamGobbler inputGobbler = null;
+    SafeProcess.InputStreamGobbler errorGobbler = null;
+    SafeProcess.InputStreamGobbler outputGobbler = null;
 
     try {       
         
-        Runtime rt = Runtime.getRuntime();
-        Process prcs = null;
+        Runtime rt = Runtime.getRuntime();      
+        
         
         // http://stackoverflow.com/questions/5283444/convert-array-of-strings-into-a-string-in-java
         logger.info("Running process: " + Arrays.toString(command_args));
-        prcs = rt.exec(this.command_args);
-
+
+        if(this.envp == null) {
+        prcs = rt.exec(this.command_args);
+        } else { // launch process using cmd str with env params        
+
+        if(this.dir == null) {
+            logger.info("\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));
+            prcs = rt.exec(this.command_args, this.envp, this.dir);
+        }
+        }
+
+        logger.info("### Before creating ProcessInGobbler");
+
+       
         // send inputStr to process. The following constructor can handle inputStr being null
-        SafeProcess.OutputStreamGobbler inputGobbler = 
+        inputGobbler = // WriterToProcessInputStream
         new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), this.inputStr);
         
+        logger.info("### Before creating ProcessErrGobbler");
+        
         // monitor for any error messages
-            SafeProcess.InputStreamGobbler errorGobbler 
+            errorGobbler // ReaderFromProcessOutputStream
         = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), splitStdOutputNewLines);
-            
+
+        logger.info("### Before creating ProcessOutGobbler");
+
             // monitor for the expected std output line(s)
-            SafeProcess.InputStreamGobbler outputGobbler
+            outputGobbler
         = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), splitStdErrorNewLines);
 
-            // kick them off
+        logger.info("### Before setting handlers on ProcessGobblers");
+                    
+        // register line by line handlers, if any were set, for the process stderr and stdout streams
+        if(this.outLineByLineHandler != null) {
+        outputGobbler.setLineByLineHandler(this.outLineByLineHandler);
+        }
+        if(this.errLineByLineHandler != null) {
+        errorGobbler.setLineByLineHandler(this.errLineByLineHandler);
+        }
+        if(this.exceptionHandler != null) {
+        inputGobbler.setExceptionHandler(this.exceptionHandler);
+        }       
+
+        logger.info("### Before streamgobblers.start()");
+
+            // kick off the stream gobblers
             inputGobbler.start();
             errorGobbler.start();
             outputGobbler.start();
+
+        logger.info("### After streamgobblers.start() - before waitFor");
                                     
             // any error???
-            this.process_exitValue = prcs.waitFor();
-            //System.out.println("ExitValue: " + exitVal); 
+            this.exitValue = prcs.waitFor(); // can throw an InterruptedException if process did not terminate              
+            logger.info("ExitValue: " + exitValue); 
+
+        logger.info("### Before streamgobblers.join()");
 
         // From the comments of 
         // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
         // To avoid running into nondeterministic failures to get the process output
-        // if there's no waiting for the threads, call join() on each Thread (StreamGobbler) object:
-        outputGobbler.join();
+        // 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."
+        outputGobbler.join(); 
         errorGobbler.join();
-        inputGobbler.join();
-        
+        inputGobbler.join(); 
+        
+        logger.info("### After streamgobblers.join()");
+
         // set the variables the code that created a SafeProcess object may want to inspect
         this.outputStr = outputGobbler.getOutput();
@@ -120 +187 @@
 
         // the calling code should handle errorStr, not us, so can leave out the following code
-        if(!this.errorStr.equals("")) {
-        logger.info("*** Process errorstream: \n" + this.errorStr + "\n****");
-        System.err.println("*** Process errorstream: \n" + this.errorStr + "\n****");
-        }
+        /*
+          if(!this.errorStr.equals("")) {
+          logger.info("*** Process errorstream: \n" + this.errorStr + "\n****");
+          //System.err.println("*** Process errorstream: \n" + this.errorStr + "\n****");
+          }
+        */
+        
+        // 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;
        
     } catch(IOException ioe) {
-        logger.error("IOexception: " + ioe.getMessage());
-        System.err.println("IOexception " + ioe.getMessage());
-        ioe.printStackTrace();
+        logger.error("IOexception: " + ioe.getMessage(), ioe);
+        //System.err.println("IOexception " + ioe.getMessage());
+        //ioe.printStackTrace();
+        if(exceptionHandler != null) exceptionHandler.gotException(ioe);
     } catch(InterruptedException ie) {
-        logger.error("Process InterruptedException: " + ie.getMessage());
-        System.err.println("Process InterruptedException " + ie.getMessage());
-        ie.printStackTrace();
-    }
-
-    }
-
-    
-
-//**************** Inner class definitions copied from GLI **********//
+        logger.error("Process InterruptedException: " + ie.getMessage(), ie);
+        //System.err.println("Process InterruptedException " + ie.getMessage());
+        //ie.printStackTrace();
+        if(exceptionHandler != null) exceptionHandler.gotException(ie);
+
+        // 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?
+    } 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();
+        }
+    }
+
+    }
+    
+
+//**************** Inner class definitions (stream gobblers copied from GLI) **********//
 // Static inner classes can be instantiated without having to instantiate an object of the outer class first
 
-    
+// Can have public static interfaces too,
+// see http://stackoverflow.com/questions/71625/why-would-a-static-nested-interface-be-used-in-java
+// Implementors need to take care that the implementations are thread safe
+// http://stackoverflow.com/questions/14520814/why-synchronized-method-is-not-included-in-interface
+public static interface ExceptionHandler {
+
+    // SHOULD I DECLARE THIS SYNCHRONIZED?
+    // It ends up being thread safe for the particular instance I'm using it for, but that doesn't
+    // make it future proof...
+    public void gotException(Exception e);
+}
+
 // When reading from a process' stdout or stderr stream, you can create a LineByLineHandler
 // to do something on a line by line basis, such as sending the line to a log
-// Can have public static interfaces too,
-// see http://stackoverflow.com/questions/71625/why-would-a-static-nested-interface-be-used-in-java
-public static interface ProcessLineByLineHandler {
-
+public static interface LineByLineHandler {
     public void gotLine(String line);
+    public void gotException(Exception e); // for when an exception occurs instead of getting a line
 }
+
 
 // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
@@ -160 +280 @@
     StringBuffer outputstr = new StringBuffer();
     boolean split_newlines = false;
-    ProcessLineByLineHandler lineByLineHandler = null;
+    LineByLineHandler lineByLineHandler = null;
     
     public InputStreamGobbler(InputStream is)
@@ -174 +294 @@
     }
     
-    public void setLineByLineHandler(ProcessLineByLineHandler lblHandler) {
+    public void setLineByLineHandler(LineByLineHandler lblHandler) {
     lineByLineHandler = lblHandler;
     }
+
 
     public void run()
@@ -184 +305 @@
         br = new BufferedReader(new InputStreamReader(is, "UTF-8"));
         String line=null;
-        while ( (line = br.readLine()) != 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.");
+            break; // will go to finally block
+        }
+
         //System.out.println("@@@ GOT LINE: " + line);
         outputstr.append(line);
         if(split_newlines) {
-            outputstr.append("\n");
+            outputstr.append(Misc.NEWLINE); // "\n" is system dependent (Win must be "\r\n")
         }
 
         if(lineByLineHandler != null) { // let handler deal with newlines
             lineByLineHandler.gotLine(line);
-        }
-        
+        }       
         }
     } catch (IOException ioe) {
-        ioe.printStackTrace();  
+        logger.error("Exception when reading from a process' stdout/stderr stream: ", ioe);
+        if(lineByLineHandler != null) lineByLineHandler.gotException(ioe);
+        //ioe.printStackTrace();  
     } finally {
         SafeProcess.closeResource(br);
@@ -212 +340 @@
 // This class is used in FormatConversionDialog to properly write to the inputstream of a Process
 // Process.getOutputStream()
-public class OutputStreamGobbler extends Thread
+public static class OutputStreamGobbler extends Thread
 {
     OutputStream os = null;
     String inputstr = "";
-    
+    ExceptionHandler exceptionHandler = null;
+
     public OutputStreamGobbler(OutputStream os, String inputstr)
     {
@@ -223 +352 @@
     }
     
+    public void setExceptionHandler(ExceptionHandler eHandler) {
+    exceptionHandler = eHandler;
+    }
+
     public void run()
-    {
-    
+    {   
     if (inputstr == null) {
         return;
@@ -251 +383 @@
         */
     } catch (IOException ioe) {
-        ioe.printStackTrace();  
+        logger.error("Exception writing to SafeProcess' inputstream: ", ioe);       
+        //ioe.printStackTrace();
+
+        if (this.exceptionHandler != null) this.exceptionHandler.gotException(ioe);
+        
     } finally {
         SafeProcess.closeResource(osw);
@@ -264 +400 @@
     // http://docs.oracle.com/javase/tutorial/essential/exceptions/finally.html
     // http://stackoverflow.com/questions/481446/throws-exception-in-finally-blocks
-    public static void closeResource(Closeable resourceHandle) {
+    public static boolean closeResource(Closeable resourceHandle) {
+    boolean success = false;
     try {
         if(resourceHandle != null) {
         resourceHandle.close();
         resourceHandle = null;
+        success = true;
         }
     } catch(Exception e) {
-        System.err.println("Exception closing resource: " + e.getMessage());
-        e.printStackTrace();
+        logger.error("Exception closing resource: ", e);
+        //System.err.println("Exception closing resource: " + e.getMessage());
+        //e.printStackTrace();
         resourceHandle = null;
-    }
-    }
-
-    public static void closeProcess(Process prcs) {
+        success = false;
+    } finally {
+        return success;
+    }
+    }
+
+    public static boolean closeProcess(Process prcs) {
+    boolean success = true;
     if( prcs != null ) { 
-        closeResource(prcs.getErrorStream()); 
-        closeResource(prcs.getOutputStream()); 
-        closeResource(prcs.getInputStream()); 
+        success = success && closeResource(prcs.getErrorStream()); 
+        success = success && closeResource(prcs.getOutputStream()); 
+        success = success && closeResource(prcs.getInputStream()); 
         prcs.destroy(); 
-    } 
+    }
+    return success;
     }