Changeset 31638


Ignore:
Timestamp:
05/01/17 18:23:39 (4 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:
4 edited

Legend:

Unmodified
Added
Removed
  • main/trunk/gli/src/org/greenstone/gatherer/collection/CollectionManager.java

    r31190 r31638  
    246246    GShell shell = new GShell(command_parts, GShell.BUILD, BUILDING, this, build_monitor, GShell.GSHELL_BUILD);
    247247    shell.addGShellListener(Gatherer.g_man.create_pane);
     248    Gatherer.g_man.create_pane.setGShell(shell); // allow the create pane to call shell.cancel()
    248249    shell.addGShellListener(Gatherer.g_man.format_pane);
    249250    shell.start();
     
    367368        GShell shell = new GShell(sched_parts, GShell.SCHEDULE, SCHEDULING, this, schedule_monitor, GShell.GSHELL_SCHEDULE);
    368369    shell.addGShellListener(Gatherer.g_man.create_pane);
     370    Gatherer.g_man.create_pane.setGShell(shell); // allow the create pane to call shell.cancel()
    369371    shell.addGShellListener(Gatherer.g_man.format_pane);
    370372    shell.start();
     
    12271229    //shell.setEventProperty("is_incremental", Boolean.toString(is_incremental));
    12281230    shell.addGShellListener(Gatherer.g_man.create_pane);
     1231    Gatherer.g_man.create_pane.setGShell(shell); // allow the create pane to call shell.cancel()
    12291232        shell.addGShellListener(Gatherer.g_man.format_pane);
    12301233    shell.start();
  • main/trunk/gli/src/org/greenstone/gatherer/gui/CreatePane.java

    r29711 r31638  
    156156    public String homepage = null;
    157157
     158    /** Access to the GShell object that this CreatePane listens to events for.
     159     * A handle to the GShell is needed order to interrupt any processes the GShell runs
     160     * when the user cancels a build operation.
     161    */
     162    private GShell shell = null;
     163
    158164    /** The constructor creates important helper classes, and initializes all the components.
    159165     * @see org.greenstone.gatherer.collection.CollectionManager
     
    291297    }
    292298
     299    public void setGShell(GShell shell) {
     300    this.shell = shell;
     301    }
    293302
    294303    public void destroy() {
     
    840849    private class CancelButtonListener
    841850    implements ActionListener {
    842     /** 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.
     851    /** This method attempts to cancel the current GShell task. It does this by first telling CollectionManager not to carry out any further action.
     852     * 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.
     853     * 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.
    843854     * @param event An <strong>ActionEvent</strong> who, thanks to the power of object oriented programming, we don't give two hoots about.
    844855     * @see org.greenstone.gatherer.Gatherer
     
    867878        build_monitor.setStop(true);
    868879        build_monitor.reset();
     880        // Tell the GShell to cleanly stop running its external process
     881        if(CreatePane.this.shell != null) {
     882        // We can call GShell.cancel() even if the GShell thread is blocking when running a process,
     883        // because this CreatePane is running in its own separate GUI thread. This is because the
     884        // process blocking call (process.waitFor()) and cancel() are not sequential operations.
     885        CreatePane.this.shell.cancel();
     886        }
    869887
    870888        // Remove the collection lock.
  • main/trunk/gli/src/org/greenstone/gatherer/shell/GShell.java

    r26574 r31638  
    5656import org.greenstone.gatherer.metadata.DocXMLFileManager;
    5757import org.greenstone.gatherer.remote.RemoteGreenstoneServer;
    58 import org.greenstone.gatherer.util.StaticStrings;
     58import org.greenstone.gatherer.util.SafeProcess;
     59import org.greenstone.gatherer.util.StaticStrings;
    5960import org.greenstone.gatherer.util.Utility;
    6061
     
    6364 */
    6465public class GShell
    65     extends Thread {
     66    extends Thread implements SafeProcess.ExceptionHandler {
    6667    /** A flag used to determine if this process has been asked to cancel. */
    6768    private boolean cancel = false;
     
    8485    private String commandOutput = null;
    8586
     87    /** The process safely run in this GShell. Make sure to set to null when done with. */
     88    SafeProcess prcs = null;
     89
    8690    /** Elements in process type enumeration. */
    8791    static final public int BUILD = 0;
     
    113117    static public String GSHELL_FEDORA_COLDELETE = "gshell_fedora_col_delete";
    114118
    115     /** 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
    116      * @param process the Process to test
    117      * @return true if it is still executing, false otherwise
    118      */
    119     static public boolean processRunning(Process process) {
    120     boolean process_running = false;
    121 
    122     try {
    123         process.exitValue(); // This will throw an exception if the process hasn't ended yet.
    124     }
    125     catch(IllegalThreadStateException itse) {
    126         process_running = true;
    127     }
    128     catch(Exception exception) {
    129         DebugStream.printStackTrace(exception);
    130     }
    131     return process_running;
    132     }
    133119
    134120    /** Constructor gatherer all the data required to create a new process, and emit meaningfull messages.
     
    236222
    237223
    238     private void runLocal(String[] args, BufferedOutputStream bos)
     224    // old_runLocal uses a potentially unsafe way of running a process and an inefficient way of reading
     225    // from the process stdout and stderr streams. Replaced by new runLocal() which uses SafeProcess and
     226    // the new CustomProcessHandler inner class instantiated for each of the 2 process output streams.
     227    private void old_runLocal(String[] args, BufferedOutputStream bos)
    239228    {
    240229    try {
     
    260249        StringBuffer stdline_buffer = new StringBuffer();
    261250   
    262         while(/*type != GShell.NEW &&*/ processRunning(prcs) && !hasSignalledStop()) {
     251        while(/*type != GShell.NEW &&*/ SafeProcess.processRunning(prcs) && !hasSignalledStop()) {
    263252        // Hopefully this doesn't block if the process is trying to write to STDOUT.
    264253        if((eisr!=null) && eisr.ready()) {
     
    348337    }
    349338   
     339    private void runLocal(String[] args, BufferedOutputStream bos)
     340    {
     341    // in case we stop between import and build, let's not launch further processes
     342    // that only pressing cancel, which results in interrupts, can subsequently stop.
     343    if(hasSignalledStop()) {
     344        return;
     345    }
     346
     347    String command = "";
     348    for(int i = 0; i < args.length; i++) {
     349        command = command + args[i] + " ";
     350    }
     351   
     352    ///ystem.err.println("Command: " + command);
     353    fireMessage(type, Dictionary.get("GShell.Command") + ": " + command, status, null);
     354   
     355    prcs = new SafeProcess(args);
     356    SafeProcess.CustomProcessHandler processOutHandler
     357        = new SynchronizedProcessHandler(bos, SafeProcess.STDOUT);
     358    SafeProcess.CustomProcessHandler processErrHandler
     359        = new SynchronizedProcessHandler(bos, SafeProcess.STDERR);
     360   
     361    prcs.setExceptionHandler(this);
     362    int exitValue = prcs.runProcess(null, processOutHandler, processErrHandler); // use default procIn handling
     363   
     364    if(exitValue == 0) {
     365        System.err.println("*** Exited normally");
     366        status = OK;
     367        fireMessage(type, typeAsString(type) + "> " + Dictionary.get("GShell.Success"), status, null);
     368    }
     369    else {
     370        System.err.println("*** Exited abnormally with exit value " + exitValue);
     371        status = ERROR;
     372        fireMessage(type, typeAsString(type) + "> " + Dictionary.get("GShell.Failure"), status, null);
     373    }
     374   
     375    prcs = null;
     376    }
     377
     378    /** When cancel is called on this GShell thread from a separate thread,
     379     * This GShell will safely terminate any process it's currently running (by interrupting it)
     380     * and will set this.prcs to null at the end. */
     381    public void cancel() {
     382    if(prcs != null) { //GShell is running a process, so interrupt the GShell/SafeProcess thread
     383
     384        SafeProcess.log("********** HAS SIGNALLED STOP. INTERRUPTING THE GSHELL/SAFEPROCESS THREAD");
     385
     386        this.interrupt(); // interrupt this thread which is running the SafeProcess prcs
     387        // this will propagate the CANCEL status
     388        prcs = null;       
     389    }
     390    hasSignalledStop(); // synchronized. Will set status to CANCELLED in thread-safe manner.   
     391    }
    350392   
    351393
     
    589631     * @return A <strong>boolean</strong> indicating if the user wanted to stop.
    590632     */
    591     public boolean hasSignalledStop() {
     633    public synchronized boolean hasSignalledStop() {
    592634    boolean has_signalled_stop = false;
    593635    if(progress != null) {
     
    639681    return name;
    640682    }
     683
     684    // From interface SafeProcess.ExceptionHandler
     685    // Called when an exception happens during the running of our perl process, as we want to set
     686    // the GShell status to ERROR.
     687    // However, exceptions when reading from our perl process' stderr and stdout streams are handled
     688    // by SynchronizedProcessHandler.gotException() below, since they happen in separate threads
     689    // from this one (the one from which the perl process is run).
     690    public void gotException(Exception e) {
     691
     692    if(e instanceof InterruptedException) {
     693        SafeProcess.log("*** InterruptedException in GShell.runLocal()");
     694        setStatus(CANCELLED); // expected exception
     695    } else {
     696        DebugStream.println("Exception in GShell.runLocal() - unexpected");
     697        DebugStream.printStackTrace(e);
     698        setStatus(ERROR); // status particularly needs to be synchronized on
     699    }
     700    }
     701
     702    public synchronized void setStatus(int newStatus) {
     703    status = newStatus;
     704    }
     705
     706    // Each instance of this class is run in its own thread by class SafeProcess.InputGobbler.
     707    // This class deals with each incoming line from the perl process' stderr or stdout streams. One
     708    // instance of this class for each stream. However, since multiple instances of this CustomProcessHandler
     709    // could be (and in fact, are) writing to the same file in their own threads, several objects, not just
     710    // the bufferedwriter object, needed to be made threadsafe.
     711    // This class also handles exceptions during the running of the perl process.
     712    // The runPerlCommand code originally would do a sendProcessStatus on each exception, so we ensure
     713    // we do that here too, to continue original behaviour. These calls are also synchronized to make their
     714    // use of the EventListeners threadsafe.
     715    protected class SynchronizedProcessHandler extends SafeProcess.CustomProcessHandler
     716    {
     717    private final BufferedOutputStream bos; // needs to be final to be able to synchronize on the shared object
     718   
     719    public SynchronizedProcessHandler(BufferedOutputStream bos, int src) {
     720        super(src); // will set this.source to STDERR or STDOUT
     721        this.bos = bos; // caller will close bw, since many more than one
     722                            // SynchronizedProcessHandlers are using it
     723    }
     724
     725    // trying to keep synchronized methods as short as possible
     726    private void logException(Exception e) {
     727        String stream = (this.source == SafeProcess.STDERR) ? "stderr" : "stdout";
     728        String msg = "Got exception when processing the perl process' " + stream + " stream.";
     729
     730        DebugStream.println(msg); // method is already synchronized
     731        DebugStream.printStackTrace(e); // method is already synchronized
     732        SafeProcess.log("*** " + msg, e);
     733
     734        GShell.this.setStatus(GShell.ERROR);
     735    }
     736
     737    private void log(String msg) {
     738        DebugStream.println(msg); // already synchro
     739        System.err.println("@@@@@ " + msg);
     740    }
     741
     742    public void run(Closeable inputStream) {
     743        InputStream is = (InputStream) inputStream;
     744
     745        BufferedReader br = null;
     746        try {       
     747
     748        br = new BufferedReader(new InputStreamReader(is, "UTF-8"));
     749        String line=null;
     750
     751        ///while ( !GShell.this.prcs.processRunning() || hasSignalledStop()) { // need to sync on prcs
     752        while ( !Thread.currentThread().isInterrupted() && (line = br.readLine()) != null ) {
     753            this.gotLine(line); // synchronized         
     754        }       
     755
     756        } catch (Exception e) {
     757        logException(e);
     758        } finally {
     759        System.err.println("*********");
     760        if(Thread.currentThread().isInterrupted()) {
     761            log("We've been asked to stop.");
     762           
     763        }
     764        SafeProcess.closeResource(br);
     765        String stream = (this.source == SafeProcess.STDERR) ? "stderr" : "stdout";
     766        System.err.println("*** In finally of " + stream + " stream");
     767        System.err.println("*********");
     768        }
     769    }
     770
     771    protected synchronized void gotLine(String line) {
     772        fireMessage(GShell.this.type,   // not final, needs synchro
     773            GShell.this.typeAsString(type) + "> " + line,
     774            GShell.this.status, // not final, needs synchro
     775            this.bos);          // needs synchro
     776    }
     777
     778    }
     779
    641780}
  • main/trunk/gli/src/org/greenstone/gatherer/util/SafeProcess.java

    r31630 r31638  
    1010import java.io.OutputStream;
    1111import java.io.OutputStreamWriter;
    12 
    1312import java.util.Arrays;
    1413
    1514import org.apache.log4j.*;
     15
     16import org.greenstone.gatherer.DebugStream;
    1617
    1718// Use this class to run a Java Process. It follows the good and safe practices at
     
    2324    public static final int STDERR = 0;
    2425    public static final int STDOUT = 1;
    25     public static final int STDIN = 2;   
     26    public static final int STDIN = 2;
     27
     28    // charset for reading process stderr and stdout streams
     29    //public static final String UTF8 = "UTF-8";   
    2630
    2731    ///static Logger logger = Logger.getLogger(org.greenstone.util.SafeProcess.class.getName());
     
    3337    private File dir = null;
    3438    private String inputStr = null;
     39    private Process process = null;
    3540
    3641    // output from running SafeProcess.runProcess()
     
    3843    private String errorStr = "";
    3944    private int exitValue = -1;
     45    //private String charset = null;
    4046
    4147    // allow callers to process exceptions of the main process thread if they want
     
    7480    public int getExitValue() { return exitValue; }
    7581
    76    
     82    //public void setStreamCharSet(String charset) { this.charset = charset; }
     83
    7784    // set any string to send as input to the process spawned by SafeProcess
    7885    public void setInputString(String sendStr) {
     
    94101    }
    95102
     103    // logger and DebugStream print commands are synchronized, therefore thread safe.
     104    public static void log(String msg) {
     105    //logger.info(msg);
     106
     107    System.err.println(msg);
     108
     109    //DebugStream.println(msg);
     110    }
     111
     112    public static void log(String msg, Exception e) { // Print stack trace on the exception
     113    //logger.error(msg, e);
     114
     115    System.err.println(msg);
     116    e.printStackTrace();
     117
     118    //DebugStream.println(msg);
     119    //DebugStream.printStackTrace(e);
     120    }
     121
     122    public static void log(Exception e) {
     123    //logger.error(e);
     124
     125    e.printStackTrace();
     126
     127    //DebugStream.printStackTrace(e);
     128    }
     129
     130    public static void log(String msg, Exception e, boolean printStackTrace) {
     131    if(printStackTrace) {
     132        log(msg, e);
     133    } else {
     134        log(msg);
     135    }
     136    }
     137
    96138    private Process doRuntimeExec() throws IOException {
    97139    Process prcs = null;
     
    99141   
    100142    if(this.command != null) {
    101         //logger.info("SafeProcess running: " + command);
    102         System.err.println("SafeProcess running: " + command_args);
     143        log("SafeProcess running: " + command);
    103144        prcs = rt.exec(this.command);
    104145    }
     
    106147       
    107148        // http://stackoverflow.com/questions/5283444/convert-array-of-strings-into-a-string-in-java
    108         //logger.info("SafeProcess running: " + Arrays.toString(command_args));
    109         System.err.println("SafeProcess running: " + Arrays.toString(command_args));
     149        log("SafeProcess running: " + Arrays.toString(command_args));
    110150       
    111151        if(this.envp == null) {
     
    114154       
    115155        if(this.dir == null) {
    116             ///logger.info("\twith: " + Arrays.toString(this.envp));
    117             ///System.err.println("\twith: " + Arrays.toString(this.envp));
     156            //log("\twith: " + Arrays.toString(this.envp));
    118157            prcs = rt.exec(this.command_args, this.envp);
    119158        } else {
    120             ///logger.info("\tfrom directory: " + this.dir);
    121             ///logger.info("\twith: " + Arrays.toString(this.envp));
    122             ///System.err.println("\tfrom directory: " + this.dir);
    123             ///System.err.println("\twith: " + Arrays.toString(this.envp));
    124             prcs = rt.exec(this.command_args, this.envp, this.dir);
     159            //log("\tfrom directory: " + this.dir);
     160            //log("\twith: " + Arrays.toString(this.envp));         
     161            prcs = rt.exec(this.command_args, this.envp, this.dir);
    125162        }
    126163        }
     
    131168
    132169    // Copied from gli's gui/FormatConversionDialog.java
    133     private int waitForWithStreams(Process prcs,
    134                    SafeProcess.OutputStreamGobbler inputGobbler,
     170    private int waitForWithStreams(SafeProcess.OutputStreamGobbler inputGobbler,
    135171                   SafeProcess.InputStreamGobbler outputGobbler,
    136172                   SafeProcess.InputStreamGobbler errorGobbler)
     
    138174    {
    139175
    140             // kick off the stream gobblers
    141             inputGobbler.start();
    142             errorGobbler.start();
    143             outputGobbler.start();
    144 
    145             // any error???
    146             this.exitValue = prcs.waitFor(); // can throw an InterruptedException if process did not terminate
    147 
    148             ///logger.info("Process exitValue: " + exitValue);
    149         ///System.err.println("Process exitValue: " + exitValue);
    150 
     176   
     177    // kick off the stream gobblers
     178    inputGobbler.start();
     179    errorGobbler.start();
     180    outputGobbler.start();
     181   
     182    // any error???
     183    try {
     184        this.exitValue = process.waitFor(); // can throw an InterruptedException if process did not terminate
     185    } catch(InterruptedException ie) {
     186       
     187        log("*** Process interrupted (InterruptedException). Expected to be a Cancel operation.");
     188            // don't print stacktrace: an interrupt here is not an error, it's expected to be a cancel action
     189       
     190        // propagate interrupts to worker threads here
     191        // unless the interrupt emanated from any of them in any join(),
     192        // which will be caught by caller's catch on InterruptedException.
     193        // Only if the thread that SafeProcess runs in was interrupted
     194        // should we propagate the interrupt to the worker threads.
     195        // http://stackoverflow.com/questions/2126997/who-is-calling-the-java-thread-interrupt-method-if-im-not
     196        // "I know that in JCiP it is mentioned that you should never interrupt threads you do not own"
     197        // But SafeProcess owns the worker threads, so it has every right to interrupt them
     198        // Also read http://stackoverflow.com/questions/13623445/future-cancel-method-is-not-working?noredirect=1&lq=1
     199       
     200        // http://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
     201        // http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
     202        // "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."
     203        // Does that mean that since this code implements this thread's interruption policy, it's ok
     204        // to swallow the interrupt this time and not let it propagate by commenting out the next line?
     205        //Thread.currentThread().interrupt(); // re-interrupt the thread
     206       
     207        inputGobbler.interrupt();
     208        errorGobbler.interrupt();
     209        outputGobbler.interrupt();
     210       
     211    } finally {     
     212       
     213        //log("Process exitValue: " + exitValue);
     214       
    151215        // From the comments of
    152216        // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
     
    154218        // if there's no waiting for the threads, call join() on each Thread (StreamGobbler) object.
    155219        // From Thread API: join() "Waits for this thread (the thread join() is invoked on) to die."
     220       
     221        // Wait for each of the threads to die, before attempting to destroy the process
     222        // Any of these can throw InterruptedExceptions too
     223        // and will be processed by the calling function's catch on InterruptedException.
     224        // However, no one besides us will interrupting these threads I think...
     225        // and we won't be throwing the InterruptedException from within the threads...
     226        // So if any streamgobbler.join() call throws an InterruptedException, that would be unexpected
     227
    156228        outputGobbler.join();
    157229        errorGobbler.join();
    158         inputGobbler.join(); 
    159        
    160 
    161         // set the variables the code that created a SafeProcess object may want to inspect
     230        inputGobbler.join();
     231       
     232       
     233        // set the variables that the code which created a SafeProcess object may want to inspect
    162234        this.outputStr = outputGobbler.getOutput();
    163235        this.errorStr = errorGobbler.getOutput();
     
    165237        // Since we didn't have an exception, process should have terminated now (waitFor blocks until then)
    166238        // Set process to null so we don't forcibly terminate it below with process.destroy()
    167         prcs = null;
    168 
    169         return this.exitValue;
    170     }
    171 
     239        this.process = null;       
     240    }
     241
     242    // Don't return from finally, it's considered an abrupt completion and exceptions are lost, see
     243    // http://stackoverflow.com/questions/18205493/can-we-use-return-in-finally-block
     244    return this.exitValue;
     245    }
     246
     247
     248    public synchronized boolean processRunning() {
     249    if(process == null) return false;
     250    return SafeProcess.processRunning(this.process);
     251    }
    172252
    173253    // Run a very basic process: with no reading from or writing to the Process' iostreams,
    174254    // this just execs the process and waits for it to return
    175255    public int runBasicProcess() {
    176     Process prcs = null;
    177256    try {
    178257        // 1. create the process
    179         prcs = doRuntimeExec();
     258        process = doRuntimeExec();
    180259        // 2. basic waitFor the process to finish
    181         this.exitValue = prcs.waitFor();
     260        this.exitValue = process.waitFor();
    182261
    183262       
     
    186265        exceptionHandler.gotException(ioe);
    187266        } else {
    188         //logger.error("IOException: " + ioe.getMessage(), ioe);
    189         System.err.println("IOException " + ioe.getMessage());
    190         ioe.printStackTrace();
     267        log("IOException: " + ioe.getMessage(), ioe);
    191268        }
    192269    } catch(InterruptedException ie) {
     
    194271        if(exceptionHandler != null) {
    195272        exceptionHandler.gotException(ie);
    196         } else {
    197         //logger.error("Process InterruptedException: " + ie.getMessage(), ie);
    198         System.err.println("Process InterruptedException " + ie.getMessage());
    199         ///ie.printStackTrace(); // an interrupt here is not an error, it can be a cancel action
     273        } else { // Unexpected InterruptedException, so printstacktrace
     274        log("Process InterruptedException: " + ie.getMessage(), ie);
    200275        }
    201276       
     
    203278    } finally {
    204279
    205         if( prcs != null ) {
    206         prcs.destroy(); // see runProcess() below
     280        if( process != null ) {
     281        process.destroy(); // see runProcess() below
    207282        }
    208283    }
     
    222297               CustomProcessHandler procErrHandler)
    223298    {
    224     Process prcs = null;
    225299    SafeProcess.OutputStreamGobbler inputGobbler = null;
    226300    SafeProcess.InputStreamGobbler errorGobbler = null;
     
    229303    try {
    230304        // 1. get the Process object
    231         prcs = doRuntimeExec();
     305        process = doRuntimeExec();
    232306       
    233307
     
    238312        // send inputStr to process. The following constructor can handle inputStr being null
    239313        inputGobbler = // WriterToProcessInputStream
    240             new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), this.inputStr);
     314            new SafeProcess.OutputStreamGobbler(process.getOutputStream(), this.inputStr);
    241315        } else { // user will do custom handling of process' InputStream
    242         inputGobbler = new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), procInHandler);
     316        inputGobbler = new SafeProcess.OutputStreamGobbler(process.getOutputStream(), procInHandler);
    243317        }
    244318
     
    246320        if(procErrHandler == null) {
    247321        errorGobbler // ReaderFromProcessOutputStream
    248             = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), splitStdErrorNewLines);
     322            = new SafeProcess.InputStreamGobbler(process.getErrorStream(), splitStdErrorNewLines);
    249323        } else {
    250324        errorGobbler
    251             = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), procErrHandler);
     325            = new SafeProcess.InputStreamGobbler(process.getErrorStream(), procErrHandler);
    252326        }
    253327
     
    255329        if(procOutHandler == null) {
    256330        outputGobbler
    257             = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), splitStdOutputNewLines);
     331            = new SafeProcess.InputStreamGobbler(process.getInputStream(), splitStdOutputNewLines);
    258332        } else {
    259333        outputGobbler
    260             = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), procOutHandler);
     334            = new SafeProcess.InputStreamGobbler(process.getInputStream(), procOutHandler);
    261335        }
    262336
    263337
    264338            // 3. kick off the stream gobblers
    265         this.exitValue = waitForWithStreams(prcs, inputGobbler, outputGobbler, errorGobbler);
     339        this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
    266340       
    267341    } catch(IOException ioe) {
     
    269343        exceptionHandler.gotException(ioe);
    270344        } else {
    271         //logger.error("IOexception: " + ioe.getMessage(), ioe);
    272         System.err.println("IOexception " + ioe.getMessage());
    273         ioe.printStackTrace();
    274         }
    275     } catch(InterruptedException ie) {
    276 
     345        log("IOexception: " + ioe.getMessage(), ioe);
     346        }
     347    } catch(InterruptedException ie) { // caused during any of the gobblers.join() calls, this is unexpected so print stack trace
     348       
    277349        if(exceptionHandler != null) {
    278350        exceptionHandler.gotException(ie);
     351        log("@@@@ Unexpected InterruptedException when waiting for process stream gobblers to die");
    279352        } else {
    280         //logger.error("Process InterruptedException: " + ie.getMessage(), ie);
    281         System.err.println("Process InterruptedException " + ie.getMessage());
    282         ///ie.printStackTrace(); // an interrupt here is not an error, it can be a cancel action
    283         }
    284 
    285         // propagate interrupts to worker threads here?
    286         // unless the interrupt emanated from any of them in any join()...
    287         // Only if the thread SafeProcess runs in was interrupted
    288         // do we propagate the interrupt to the worker threads.
    289         // http://stackoverflow.com/questions/2126997/who-is-calling-the-java-thread-interrupt-method-if-im-not
    290         // "I know that in JCiP it is mentioned that you should never interrupt threads you do not own"
    291         // But SafeProcess owns the worker threads, so it have every right to interrupt them
    292         // Also read http://stackoverflow.com/questions/13623445/future-cancel-method-is-not-working?noredirect=1&lq=1
    293         if(Thread.currentThread().isInterrupted()) {
    294         inputGobbler.interrupt();
    295         errorGobbler.interrupt();
    296         outputGobbler.interrupt();     
    297         }
    298 
    299         // On catchingInterruptedException, re-interrupt the thread.
    300         // This is just how InterruptedExceptions tend to be handled
    301         // See also http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
    302         // and https://praveer09.github.io/technology/2015/12/06/understanding-thread-interruption-in-java/
    303 
    304         // http://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
    305         // http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
    306         // "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."
    307         // Does that mean that since this code implements this thread's interruption policy, it's ok
    308         // to swallow the interrupt this time and not let it propagate by commenting out the next line?
    309         Thread.currentThread().interrupt(); // re-interrupt the thread - which thread? Infinite loop?
     353        log("*** Unexpected InterruptException when waiting for process stream gobblers to die:" + ie.getMessage(), ie);
     354        }
     355
     356        // see comments in other runProcess()
     357        Thread.currentThread().interrupt();
     358   
    310359    } finally {
    311360
    312         // Moved into here from GS2PerlConstructor which said
    313         // "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."
    314         // http://steveliles.github.io/invoking_processes_from_java.html
    315         // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
    316         // http://mark.koli.ch/leaky-pipes-remember-to-close-your-streams-when-using-javas-runtimegetruntimeexec       
    317         if( prcs != null ) {       
    318         prcs.destroy();
    319         }
     361        log("*** In finally of SafeProcess.runProcess(3 params)");
     362
     363        if( process != null ) {
     364        log("*** Going to call process.destroy 2");
     365        process.destroy();
     366        process = null;
     367        log("*** Have called process.destroy 2");
     368        }
     369       
    320370    }
    321371   
     
    325375    public int runProcess(LineByLineHandler outLineByLineHandler, LineByLineHandler errLineByLineHandler)
    326376    {
    327     Process prcs = null;
    328377    SafeProcess.OutputStreamGobbler inputGobbler = null;
    329378    SafeProcess.InputStreamGobbler errorGobbler = null;
     
    332381    try {
    333382        // 1. get the Process object
    334         prcs = doRuntimeExec();
     383        process = doRuntimeExec();
    335384       
    336385
     
    340389        // send inputStr to process. The following constructor can handle inputStr being null
    341390        inputGobbler = // WriterToProcessInputStream
    342         new SafeProcess.OutputStreamGobbler(prcs.getOutputStream(), this.inputStr);
     391        new SafeProcess.OutputStreamGobbler(process.getOutputStream(), this.inputStr);
    343392
    344393        // PROC ERR STREAM to monitor for any error messages or expected output in the process' stderr   
    345394        errorGobbler // ReaderFromProcessOutputStream
    346             = new SafeProcess.InputStreamGobbler(prcs.getErrorStream(), splitStdErrorNewLines);     
     395            = new SafeProcess.InputStreamGobbler(process.getErrorStream(), splitStdErrorNewLines);     
    347396            // PROC OUT STREAM to monitor for the expected std output line(s)
    348397        outputGobbler
    349         = new SafeProcess.InputStreamGobbler(prcs.getInputStream(), splitStdOutputNewLines);
     398        = new SafeProcess.InputStreamGobbler(process.getInputStream(), splitStdOutputNewLines);
    350399
    351400
     
    360409
    361410            // 4. kick off the stream gobblers
    362         this.exitValue = waitForWithStreams(prcs, inputGobbler, outputGobbler, errorGobbler);
     411        this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
    363412       
    364413    } catch(IOException ioe) {
     
    366415        exceptionHandler.gotException(ioe);
    367416        } else {
    368         //logger.error("IOexception: " + ioe.getMessage(), ioe);
    369         System.err.println("IOexception " + ioe.getMessage());
    370         ioe.printStackTrace();
    371         }
    372     } catch(InterruptedException ie) {
    373 
     417        log("IOexception: " + ioe.getMessage(), ioe);       
     418        }
     419    } catch(InterruptedException ie) { // caused during any of the gobblers.join() calls, this is unexpected so log it
     420       
    374421        if(exceptionHandler != null) {
    375422        exceptionHandler.gotException(ie);
     423        log("@@@@ Unexpected InterruptedException when waiting for process stream gobblers to die");
    376424        } else {
    377         //logger.error("Process InterruptedException: " + ie.getMessage(), ie);
    378         System.err.println("Process InterruptedException " + ie.getMessage());
    379         ///ie.printStackTrace(); // an interrupt here is not an error, it can be a cancel action
    380         }
    381 
    382         // propagate interrupts to worker threads here?
    383         // unless the interrupt emanated from any of them in any join()...
    384         // Only if the thread SafeProcess runs in was interrupted
    385         // do we propagate the interrupt to the worker threads.
    386         // http://stackoverflow.com/questions/2126997/who-is-calling-the-java-thread-interrupt-method-if-im-not
    387         // "I know that in JCiP it is mentioned that you should never interrupt threads you do not own"
    388         // But SafeProcess owns the worker threads, so it have every right to interrupt them
    389         // Also read http://stackoverflow.com/questions/13623445/future-cancel-method-is-not-working?noredirect=1&lq=1
    390         if(Thread.currentThread().isInterrupted()) {
    391         inputGobbler.interrupt();
    392         errorGobbler.interrupt();
    393         outputGobbler.interrupt();     
    394         }
    395 
    396         // On catchingInterruptedException, re-interrupt the thread.
     425        log("*** Unexpected InterruptException when waiting for process stream gobblers to die: " + ie.getMessage(), ie);
     426        }
     427        // We're not causing any interruptions that may occur when trying to stop the worker threads
     428        // So resort to default behaviour in this catch?
     429        // "On catching InterruptedException, re-interrupt the thread."
    397430        // This is just how InterruptedExceptions tend to be handled
    398431        // See also http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
    399432        // and https://praveer09.github.io/technology/2015/12/06/understanding-thread-interruption-in-java/
    400 
    401         // http://stackoverflow.com/questions/3976344/handling-interruptedexception-in-java
    402         // http://stackoverflow.com/questions/4906799/why-invoke-thread-currentthread-interrupt-when-catch-any-interruptexception
    403         // "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."
    404         // Does that mean that since this code implements this thread's interruption policy, it's ok
    405         // to swallow the interrupt this time and not let it propagate by commenting out the next line?
    406433        Thread.currentThread().interrupt(); // re-interrupt the thread - which thread? Infinite loop?
     434   
    407435    } finally {
    408436
    409         // Moved into here from GS2PerlConstructor which said
    410         // "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."
     437        // Moved into here from GS2PerlConstructor and GShell.runLocal() which said
     438        // "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!)"
    411439        // http://steveliles.github.io/invoking_processes_from_java.html
    412440        // http://www.javaworld.com/article/2071275/core-java/when-runtime-exec---won-t.html?page=2
    413441        // http://mark.koli.ch/leaky-pipes-remember-to-close-your-streams-when-using-javas-runtimegetruntimeexec       
    414         if( prcs != null ) {       
    415         prcs.destroy();
    416         }
     442        if( process != null ) {
     443        log("*** Going to call process.destroy 1");
     444        process.destroy();
     445        process = null;
     446        log("*** Have called process.destroy 1");
     447        }
     448        log("*** In finally of SafeProcess.runProcess(2 params)");
    417449    }
    418450   
     
    452484    protected CustomProcessHandler(int src) {
    453485    this.source = src; // STDERR or STDOUT or STDIN
     486
     487    // modify threadname to prefix stream src (stdin/stderr/stdout)
     488    // Useful for debugging if thread is named clearly
     489    String stream;
     490    switch(src) {
     491    case SafeProcess.STDERR:
     492       stream = "stderr";
     493    case SafeProcess.STDOUT:
     494        stream = "stdout";
     495    default:
     496        stream = "stdin";
     497    }
     498    Thread.currentThread().setName(stream + Thread.currentThread().getName());
    454499    }
    455500   
     
    487532    public InputStreamGobbler(InputStream is)
    488533    {
     534    super("InputStreamGobbler"); // thread name
    489535    this.is = is;
    490536    this.split_newlines = false;
     
    493539    public InputStreamGobbler(InputStream is, boolean split_newlines)
    494540    {
     541    super("InputStreamGobbler"); // thread name
    495542    this.is = is;
    496543    this.split_newlines = split_newlines;
     
    499546    public InputStreamGobbler(InputStream is, CustomProcessHandler customHandler)
    500547    {
     548    super("InputStreamGobbler"); // thread name
    501549    this.is = is;
    502550    this.customHandler = customHandler;
     
    514562        br = new BufferedReader(new InputStreamReader(is, "UTF-8"));
    515563        String line=null;
    516         while ( (line = br.readLine()) != null ) {
    517 
    518         if(this.isInterrupted()) { // should we not instead check if SafeProcess thread was interrupted?
    519             //logger.info("Got interrupted when reading lines from process err/out stream.");
    520             //System.err.println("InputStreamGobbler.runDefault() Got interrupted when reading lines from process err/out stream.");
    521             break; // will go to finally block
    522         }
    523 
    524         //System.out.println("@@@ GOT LINE: " + line);
     564        while ( !this.isInterrupted() && (line = br.readLine()) != null ) {
     565
     566        //log("@@@ GOT LINE: " + line);
    525567        outputstr.append(line);
    526568        if(split_newlines) {
     
    532574        }
    533575        }
     576
    534577    } catch (IOException ioe) {
    535578        if(lineByLineHandler != null) {
    536579        lineByLineHandler.gotException(ioe);
    537580        } else {
    538         //logger.error("Exception when reading from a process' stdout/stderr stream: ", ioe);
    539         System.err.println("Exception when reading from a process' stdout/stderr stream: ");
    540         ioe.printStackTrace(); 
     581        log("Exception when reading from a process' stdout/stderr stream: ", ioe);
    541582        }
    542583    } finally {
     584        log("*********");
     585        if(this.isInterrupted()) {
     586        log("We've been asked to stop.");       
     587        }
    543588        SafeProcess.closeResource(br);
     589        log("*** In finally of " + this.getName());
     590        log("*********");
    544591    }
    545592    }
     
    573620
    574621    public OutputStreamGobbler(OutputStream os) {
     622    super("OutputStreamGobbler"); // thread name
    575623    this.os = os;
    576624    }
     
    578626    public OutputStreamGobbler(OutputStream os, String inputstr)
    579627    {
     628    super("OutputStreamGobbler"); // thread name
    580629    this.os = os;
    581630    this.inputstr = inputstr;
     
    583632
    584633    public OutputStreamGobbler(OutputStream os, CustomProcessHandler customHandler) {
     634    super("OutputStreamGobbler"); // thread name
    585635    this.os = os;
    586636    this.customHandler = customHandler;
     
    592642    if (inputstr == null) {
    593643        return;
     644    }
     645
     646    // also quit if the process was interrupted before we could send anything to its stdin
     647    if(this.isInterrupted()) {
     648        log(this.getName() + " thread was interrupted.");
     649        return;
    594650    }
    595651   
     
    615671        */
    616672    } catch (IOException ioe) {
    617         //logger.error("Exception writing to SafeProcess' inputstream: ", ioe);
    618         System.err.println("Exception writing to SafeProcess' inputstream: ");
    619         ioe.printStackTrace(); 
     673        log("Exception writing to SafeProcess' inputstream: ", ioe);
    620674    } finally {
    621675        SafeProcess.closeResource(osw);
     
    635689        runCustom();
    636690    }
    637 
    638691    }   
    639692} // end static inner class OutputStreamGobbler
     
    654707        }
    655708    } catch(Exception e) {
    656         //logger.error("Exception closing resource: ", e);
    657         System.err.println("Exception closing resource: " + e.getMessage());
    658         e.printStackTrace();
     709        log("Exception closing resource: " + e.getMessage(), e);
    659710        resourceHandle = null;
    660711        success = false;
     
    675726    }
    676727
     728// Moved from GShell.java
     729    /** 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
     730     * @param process the Process to test
     731     * @return true if it is still executing, false otherwise
     732     */
     733    static public boolean processRunning(Process process) {
     734    boolean process_running = false;
     735
     736    try {
     737        process.exitValue(); // This will throw an exception if the process hasn't ended yet.
     738    }
     739    catch(IllegalThreadStateException itse) {
     740        process_running = true;
     741    }
     742    catch(Exception exception) {
     743        log(exception); // DebugStream.printStackTrace(exception);
     744    }
     745    return process_running;
     746    }
     747
    677748} // end class SafeProcess
Note: See TracChangeset for help on using the changeset viewer.