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

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.

File:

• main/trunk/gli/src/org/greenstone/gatherer/shell/GShell.java (modified) (9 diffs)

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
+    }
+
+    }
+
 }