Changeset 31692

Timestamp:

2017-05-18T20:38:56+12:00 (7 years ago)

Author:

ak19

Message:

Major changes to SafeProcess and classes of the download package, by bringing the final GLI (and final Greenstone) class DownloadJob over to use SafeProcess. Significant changes to SafeProcess: 1. Introduction of cancelRunningProcess as a new method, so that callers don't need to know how cancelling a process is implemented (as an interrupt) nor do they need to know what thread they ought to interrupt (which should be the thread that launched SafeProcess), nor do they need to know of the complexity surrounding the join() blocking call which should not be interrupted. 2. Introduction of the SafeProcess.MainHandler interface that provides methods that allow implementers to write hooks to various stages of the SafeProcess' internal process' life cycle. 3. moved process cleanUp() code into a reusable method within SafeProcess. Significant changes to DownloadJob and its associated DownloadProgressBar and DownloadScrollPane classes: 1. Unused member vars removed or commented out and those that can be declared final are now declared so, as a programming pricinple for thread safety, since DownloadJob and the other download classes will have to interact with multiple threads and could be called by different threads. 2. Replaced existing actionPerformed() and callDownload() of DownloadJob with SafeProcess and implemented the necessary Hooks in the SafeProcess.MainHandler() interface to ensure that perl is still told to terminate wget on a cancel action. 3. Replaced DownloadScrollPane's deleteDownloadJob() with a new version that now more responsively removes its DownloadProgressBar (with controls) for a DownloadJob. It's called by the SafeProcess.MainHandler interface hooks implemented by DownloadJob, so DownloadScrollPane no longer needs to wait() to be notify()ed when a process has cleaned up on premature termination by a cancel action. 4. Made the necessary methods in DownloadProgressBar synchronized for thread safety. 5. GShell now uses SafeProcess' new cancelRunningProcess() method in place of directly calling interrupt on the (GShell) thread that launched SafeProcess.

Location:

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

Files:

• DebugStream.java (modified) (1 diff)
• download/DownloadJob.java (modified) (18 diffs)
• download/DownloadProgressBar.java (modified) (11 diffs)
• download/DownloadScrollPane.java (modified) (4 diffs)
• shell/GShell.java (modified) (2 diffs)
• util/SafeProcess.java (modified) (23 diffs)

main/trunk/gli/src/org/greenstone/gatherer/DebugStream.java

@@ -58 +58 @@
 
 
-    static public boolean isDebuggingEnabled()
+    static synchronized public boolean isDebuggingEnabled()
     {
     return debugging_enabled;

main/trunk/gli/src/org/greenstone/gatherer/download/DownloadJob.java

@@ -42 +42 @@
 import java.util.*;
 import javax.swing.tree.*;
+import javax.swing.SwingUtilities;
 import org.greenstone.gatherer.Configuration;
 import org.greenstone.gatherer.DebugStream;
@@ -50 +51 @@
 import org.greenstone.gatherer.file.WorkspaceTree;
 import org.greenstone.gatherer.util.AppendLineOnlyFileDocument;
+import org.greenstone.gatherer.util.SafeProcess;
 import org.greenstone.gatherer.util.Utility;
 import org.greenstone.gatherer.cdm.Argument;
 import org.greenstone.gatherer.collection.*;
+
 /**
  * @author John Thompson, Greenstone Digital Library, University of Waikato
  * @version 2.0
+ * When modifying this class, bear in mind concurrency issues that could arise with 
+ * SafeProcess's worker threads and where synchronization may be needed to prevent such issues.
  */
 public class DownloadJob
-    implements ActionListener {
-     
-    private boolean debug;
-    private boolean higher_directories;
-    private boolean no_parents;
-    private boolean other_hosts; 
-    private boolean page_requisites; 
-    private boolean quiet;
+    implements ActionListener, SafeProcess.MainProcessHandler {
 
     private AppendLineOnlyFileDocument download_log;
@@ -71 +69 @@
     private DownloadProgressBar progress;
 
-    private int depth;
     private int previous_state;
     private int state;
 
-    private String download_url = "";
+    private SafeProcess prcs = null;
+
+    private final String download_url;
     
     //    private String current_url;
     //    private String destination;
-    private String proxy_pass;
-    private String proxy_user;
-
-    private Vector encountered_urls;
-    private Vector failed_urls;
+    private final String proxy_pass;
+    private final String proxy_user;
+
+    //private final Vector encountered_urls;
+    //private Vector failed_urls;
     private Download download;
     private DownloadScrollPane mummy;
     private HashMap download_option;
 
-    public static int COMPLETE = 0;
-    public static int PAUSED   = 1;
-    public static int RUNNING  = 2;
-    public static int STOPPED  = 3;
-
-    public static int UNKNOWN_MAX  = 0;
-    public static int DEFINED_MAX  = 1;
-    public static int UNDEFINED_MAX  = 2;
+    public static final int COMPLETE = 0;
+    public static final int PAUSED   = 1;
+    public static final int RUNNING  = 2;
+    public static final int STOPPED  = 3;
+
+    public static final int UNKNOWN_MAX  = 0;
+    public static final int DEFINED_MAX  = 1;
+    public static final int UNDEFINED_MAX  = 2;
     
     // To prematurely terminate wget, we will need to use sockets and find a free port.
@@ -103 +102 @@
     private static int nextFreePort = PORT_BASE;  // Keep track what port numbers we have checked for availability
     int port;                                     // package access. The socket port number this instance of DownloadJob will use
+                                             // only the main thread (where DownloadJob runs) modifies port, so no synching needed
     
-    private String mode = null;
+    private final String mode;
    
-    private String proxy_url;
+    private String proxy_url; // only the main thread (where DownloadJob runs) modifies this, so no synching needed
     
     /**
@@ -149 +149 @@
 
     progress = new DownloadProgressBar(this,download_url, true);
-    encountered_urls = new Vector();
-    failed_urls = new Vector();
+    //encountered_urls = new Vector();
+    //failed_urls = new Vector();
 
     previous_state = STOPPED;
@@ -167 +167 @@
     }
 
-    /** Depending on which button on the progress bar was pushed,
+    /** Depending on which button on the progress bar was pushed,
      * this method will affect the state of the DownloadJob and perhaps make
      * calls to wget.class if necessary.
@@ -173 +173 @@
      * which we must respond to.
      */
-    public void actionPerformed(ActionEvent event) {
+    public void old_actionPerformed(ActionEvent event) {
     // The stop_start_button is used to alternately start or stop the
     // job. If the current state of the job is paused then this
@@ -195 +195 @@
     }
     }
+    
+    /** Depending on which button on the progress bar was pushed,
+     * this method will affect the state of the DownloadJob and perhaps make
+     * calls to wget.class if necessary.
+     * @param event The ActionEvent fired from within the DownloadProgressBar
+     * which we must respond to.
+     * Now using synchronized methods like previous_state = getState(); instead of
+     * previous_state = state; and setState(STOPPED); instead of state = STOPPED;
+     */
+    public void actionPerformed(ActionEvent event) {
+    // The stop_start_button is used to alternately start or stop the
+    // job. If the current state of the job is paused then this
+    // restart is logically equivalent to a resume.
+    if(event.getSource() == progress.stop_start_button) {
+        previous_state = getState();
+        if (getState() == RUNNING) {
+        //setState(STOPPED);
+        stopDownload(); // cancels any running SafeProcess      
+        } else {
+        //previous_state = getState();
+        setState(RUNNING);
+        mummy.resumeThread();
+        }
+    }
+    else if (event.getSource() == progress.close_button) {
+        SafeProcess.log("@@@ Progress bar close button pressed");
+        if(getState() == RUNNING) {
+        previous_state = getState();
+        //setState(STOPPED); // do we need to do anything else to stop this? YES, we do:
+        stopDownload(); // cancels any running SafeProcess      
+        } 
+        mummy.deleteDownloadJob(this);
+    }
+    }
 
     /** Given a portnumber to check, returns true if it is available
@@ -229 +263 @@
     } 
 
-    public void callDownload() {
+    // If eschewing the use of SafeProcess, reactivate (by renaming) old_callDownload()
+    // and old_actionPerformed(), and DownloadScrollPane.java's old_deleteDownloadJob().
+    public void old_callDownload() {
 
     ArrayList command_list = new ArrayList();
@@ -537 +573 @@
     }
 
+    public void callDownload() {
+
+    ArrayList command_list= new ArrayList();
+    
+    // the following also works for client-gli if downloading is enabled (when there's a gs2build directory inside gli)
+    command_list.add(Configuration.perl_path);
+    command_list.add("-S");
+    command_list.add(LocalGreenstone.getBinScriptDirectoryPath()+"downloadfrom.pl");
+    command_list.add("-download_mode");
+    command_list.add(mode);
+    command_list.add("-cache_dir");
+    command_list.add(Gatherer.getGLIUserCacheDirectoryPath());
+    // For the purposes of prematurely terminating wget from GLI (which creates a socket
+    // as a communication channel between GLI and Perl), it is important to tell the script
+    // that we're running as GLI. Because when running from the command prompt, it should
+    // not create this socket and do the related processing.
+    command_list.add("-gli");
+    
+    ArrayList all_arg = download.getArguments(true,false);
+    for(int i = 0; i < all_arg.size(); i++) {
+        Argument argument = (Argument) all_arg.get(i);
+        if(argument.isAssigned()) {
+        command_list.add("-" + argument.getName());
+        if(argument.getType() != Argument.FLAG) {
+            command_list.add(argument.getValue());
+        }
+        }   
+    }
+
+    String [] cmd = (String []) command_list.toArray(new String[0]);
+    DebugStream.println("Download job, "+command_list);
+
+    if (previous_state == DownloadJob.COMPLETE) {
+        progress.mirrorBegun(true, true);
+    }
+    else {
+        progress.mirrorBegun(false, true);
+    }
+
+    try {
+        Runtime rt = Runtime.getRuntime();
+
+        String [] env = null;
+         
+        if (Utility.isWindows()) {
+                prcs = new SafeProcess(cmd);
+        }
+        else {
+        if (proxy_url != null && !proxy_url.equals("")) {           
+            // Specify proxies as environment variables
+            // Need to manually specify GSDLHOME and GSDLOS also
+            env = new String[4];
+                    proxy_url = proxy_url.replaceAll("http://","");
+            env[0] = "http_proxy=http://"+proxy_url;
+            env[1] = "ftp_proxy=ftp://"+proxy_url;
+            env[2] = "GSDLHOME=" + Configuration.gsdl_path;
+            env[3] = "GSDLOS=" + Gatherer.client_operating_system;
+
+            prcs = new SafeProcess(cmd, env, null);
+        } 
+        else if(Gatherer.isGsdlRemote && Gatherer.isDownloadEnabled) {
+            // Not Windows, but running client with download panel
+            // Need to manually specify GSDLHOME and GSDLOS
+            env = new String[2];
+            env[0] = "GSDLHOME=" + Configuration.gsdl_path;
+            env[1] = "GSDLOS=" + Gatherer.client_operating_system;
+
+            prcs = new SafeProcess(cmd, env, null);
+        }
+        else {
+            // Will inherit the GLI's environment, with GSDLHOME and GSDLOS set
+            prcs = new SafeProcess(cmd);
+        }
+        }
+        //System.out.println(newcmd);
+        prcs.setMainHandler(this); // attach handler to clean up before and after process.destroy()
+                                   // for which DownloadJob implements SafeProcess.MainProcessHandler 
+
+        // To be able to stop Wget, we use sockets to communicate with the perl process that launched wget
+        if (mode.equals("Web") || mode.equals("MediaWiki")) { // wget download modes other than OAI
+        
+        // Need to find an available (unused) port within the range we're looking for to pass it
+        // the Perl child process, so that it may set up a listening ServerSocket at that port number
+        try {
+            boolean foundFreePort = false;
+            for(int i = 0; i < PORT_BLOCK_SIZE; i++) {
+
+            if(isPortAvailable(nextFreePort)) {
+                foundFreePort = true;
+                break;
+                
+            } else {
+                incrementNextFreePort();
+            }
+            }
+            
+            if(foundFreePort) {
+            // Free port number currently found becomes the port number of the socket that this
+            // DownloadJob instance will be connecting to when the user wants to prematurely stop Wget.
+            this.port = nextFreePort;
+            incrementNextFreePort(); //// Necessary?
+            
+            } else {
+            throw new Exception("Cannot find an available port in the range " 
+                        + PORT_BASE + "-" + (PORT_BASE+PORT_BLOCK_SIZE)
+                        + "\nwhich is necessary for forcibly terminating wget.");
+            }
+
+            // Communicate the chosen port for this DownloadJob instance to the perl process, so
+            // that it can set up a ServerSocket at that port to listen for any signal to terminate wget
+            //OutputStream os = prcs.getOutputStream();
+            String p = ""+this.port+"\n";
+            System.err.println("Portnumber found: " + p);
+
+            prcs.setInputString(p);
+          
+        } catch(Exception ex) {
+            System.err.println("Sent available portnumber " + this.port + " to process' outputstream.\nBut got exception: " + ex);
+        }
+        }
+
+        ProcessErrHandler errHandler = new ProcessErrHandler(); // meaningful output comes from prcs stderr
+        ProcessOutHandler outHandler = new ProcessOutHandler(); // debugging output comes from prcs' stdout
+
+        int exitVal = prcs.runProcess(null, outHandler, errHandler);
+        
+        // if prcs is interrupted (cancelled) during the blocking runProcess() call,
+        // as happens on state == STOPPED, then
+        // beforeWaitingForStreamsToEnd() is called before the process' worker threads come to a halt
+        // and afterStreamsEnded() is called when the process' worker threads have halted,
+        // beforeProcessDestroy() is called before the process is destroyed,
+        // and afterProcessDestroy() is called after the proc has been destroyed.
+        // If when beforeWaitingForStreamsEnd() stage the perl was still running but had been 
+        // told to stop, then the beforeWaitingForStreamsEnd() method will make sure to communicate
+        // with the perl process over a socket and send it the termination message, 
+        // which will also kill any runnning wget that perl launched.
+        // In that case, destroy() is actually called on the process at last.
+
+    }
+    catch (Exception ioe) {
+        SafeProcess.log(ioe);
+        DebugStream.printStackTrace(ioe);
+    }
+
+    // now the process is done, we can at last null it
+    prcs = null;
+
+    // If we've got to here and the state isn't STOPPED then the
+    // job is complete.
+    if(getState() == DownloadJob.RUNNING) {
+        progress.mirrorComplete();
+        previous_state = getState();
+        setState(DownloadJob.COMPLETE);     
+    }   
+
+    SafeProcess.log("@@@@ DONE callDownload()");
+    
+    /*
+    // Regardless of whether state==STOPPED or ends up being COMPLETE, the process is at an end now.
+    // Notify the DownloadScrollPane which is waiting on this job to complete that we are ready 
+    synchronized(this) {
+        System.err.println("**************** Notifying download scrollpane");
+        this.notify();
+    }
+    */  
+
+    // refresh the workspace tree
+    Gatherer.g_man.refreshWorkspaceTree(WorkspaceTree.DOWNLOADED_FILES_CHANGED);
+    }
+
+    private synchronized boolean isStopped() { return state == STOPPED; }
+
+    // called when the user cancelled the download and we're told to stop both our external perl process
+    // and the wget process that it in turn launched
+    public void stopDownload() {    
+    if(prcs != null) {      
+        SafeProcess.log("@@@ Going to interrupt the SafeProcess...");       
+
+        // Whether a process ends naturally or is prematurely ended, beforeWaitingForStreamsToEnd()
+        // will be called. We've hooked this in to calling tellPerlToTerminateWget() only if the
+        // process is still running when cancel is pressed, but not when it's naturally terminated.
+        boolean hadToSendInterrupt = prcs.cancelRunningProcess(); // returns false if it was already terminating/terminated, true if interrupt sent
+
+
+        /*
+          // if process terminating naturally but waiting for process' worker threads to join(),
+          // shall we just remove the progress bar display for this download?
+          // If so, do this section in place of the 2 calls to progress.enableCancelJob(boolean) below
+        if(!hadToSendInterrupt && SwingUtilities.isEventDispatchThread()) {
+        if(getState() == DownloadJob.RUNNING) {
+            progress.mirrorComplete();
+            previous_state = getState();
+            setState(DownloadJob.COMPLETE);     
+        }
+        mummy.deleteCurrentDownloadJob(this); // why wait for the cleanup which can't be interrupted anyway?
+        }
+        */
+    } else {
+        System.err.println("@@@@ No process to interrupt");
+    }
+
+    //setState(STOPPED); // would set it to stop on cancel, even if it already naturally terminated
+    
+    }
+    
+//*********** START of implementing interface Safeprocess.MainProcessHandler
+    // before and after processDestroy only happen when interrupted AND terminatePerlScript=true
+    public void beforeProcessDestroy() {}    
+    public void afterProcessDestroy() {}
+
+    // after blocking call on closing up streamgobbler worker threads that happens
+    // upon natural termination or interruption of process' main body/thread.
+    // if not overriding, then return the parameter forciblyTerminating as-is
+    public boolean afterStreamsEnded(boolean forciblyTerminating) { return forciblyTerminating; }
+
+    // called after the SafeProcess has fully terminated (naturally or via process.destroy())
+    // and has been cleaned up
+    public void doneCleanup(boolean wasForciblyTerminated) {
+    // let the user know they can cancel again now cleanup phase is done
+    progress.enableCancelJob(true);
+    
+    if(wasForciblyTerminated) {
+        setState(STOPPED); // sets it to stop only if process truly was prematurely terminated, not merely
+        // if the cancel button was clicked when it had already naturally terminated
+
+        // we're now ready to remove the display of the until now running job
+        // from the download progress bar interface
+        mummy.deleteCurrentDownloadJob(this); 
+    } /*else {
+        // If we've got to here and the state isn't STOPPED then the
+        // job is complete.
+        System.err.println("**************** NOT Notifying download scrollpane");
+        if(getState() == DownloadJob.RUNNING) {
+        progress.mirrorComplete();
+        previous_state = getState();
+        setState(DownloadJob.COMPLETE);     
+        }
+    }
+    
+    // Regardless of whether state==STOPPED or ends up being COMPLETE, the process is at an end now.
+    // Notify the DownloadScrollPane which is waiting on this job to complete that we are ready 
+    synchronized(this) {
+        System.err.println("**************** Notifying download scrollpane");
+        this.notify();
+        }*/
+
+    }
+
+    // before blocking call of ending streamgobbler worker threads that happens
+    // after process' main body/thread has naturally terminated or been interrupted
+    public boolean beforeWaitingForStreamsToEnd(boolean forciblyTerminating) {
+    // let the user know they can't cancel during cleanup phase
+    progress.enableCancelJob(false);
+
+    SafeProcess.log("**** in beforeWaitingForStreamsToEnd()");
+
+    // state would not be STOPPED if cancel was pressed after the process naturally terminated anyway
+    // in that case we don't need to send perl the signal to terminate WGET
+    if(!forciblyTerminating) { //if(!isStopped()) {
+        SafeProcess.log("*** Process not (yet) cancelled/state not (yet) stopped");
+        SafeProcess.log("*** But process has naturally terminated (process streams are being closed before any interruption signal can be received), so won't be destroying process even on interrupt");
+        return false; // for us to be in this method at all with forciblyTerminating being false 
+            // means the process is already naturally terminating, so don't unnaturally destroy it
+    }
+    
+    // else the process is still running and we've been told to stop, so tell perl to stop wget first
+    // (so that process destroy can then be called thereafter)
+    return tellPerlToTerminateWget();
+    }
+//*********** END of implementing interface Safeprocess.MainProcessHandler
+    
+    public boolean tellPerlToTerminateWget() {
+    SafeProcess.log("**** in tellPerlToTerminateWget()");
+    
+    boolean terminatePerlScript = true;
+    
+    // When GLI is working with wget-based download modes other than OAI (MediaWiki and Web 
+    // download) and the STOP button has been pressed, wget needs to be prematurely terminated.
+    // Only wget download modes Web and MediaWiki require the use of sockets to communicate
+    // with the perl script in order to get wget to terminate. Other download modes, including
+    // wgetdownload mode OAI, can terminate in the traditional manner: close process inputstream 
+    // and kill perl process. OAI launches many wgets. So that when the perl process is terminated,
+    // the currently running wget will finish off but other wgets are no longer launched. 
+    if((mode.equals("Web") || mode.equals("MediaWiki"))) { 
+        SafeProcess.log("@@@ Socket communication to end wget");
+        // create a socket to the perl child process and communicate the STOP message
+        Socket clientSocket = null;
+        BufferedReader clientReader = null;
+        OutputStream os = null;
+        
+        if(clientSocket == null) {
+        try {
+            clientSocket = new Socket("localhost", this.port); // connect to the port chosen for this DownloadJob instance
+            
+            clientReader = new BufferedReader(new InputStreamReader(clientSocket.getInputStream()));
+            String response = clientReader.readLine(); // see if we've been connected
+            System.err.println("Communicating with perl download script on port " + this.port 
+                       + "\nGot response from perl: " + response); 
+            
+            // Send the STOP signal
+            os = clientSocket.getOutputStream();
+            String message = "<<STOP>>\n";
+            os.write(message.getBytes());
+            response = clientReader.readLine(); // see whether the stop signal has been received
+            System.err.println("GLI sent STOP signal to perl to terminate wget." 
+                       + "\nGot response from perl: " + response);
+            
+            response = clientReader.readLine(); // see whether the perl script is ready to be terminated
+            System.err.println("Got another response from perl: " + response);      
+            
+            if(response == null) { // why? Is it because the process has already terminated naturally if response is null?
+            terminatePerlScript = false;
+            }
+        } catch(IOException ex) {
+            if(ex instanceof IOException && ex.getMessage().indexOf("Connection refused") != -1) {
+            terminatePerlScript = false; // no socket listening on other end because process ended
+            System.err.println("Tried to communicate through client socket - port " + this.port + ", but the process seems to have already ended naturally");
+            } else {
+            System.err.println("Tried to communicate through client socket - port " + this.port + ", but got exception: " + ex);
+            }
+
+        } catch(Exception ex) {
+            System.err.println("Tried to open client socket, but got exception: " + ex);
+        } finally {
+            SafeProcess.closeResource(os);
+            SafeProcess.closeResource(clientReader);
+            SafeProcess.closeSocket(clientSocket); // close the clientSocket (the Perl end will close the server socket that Perl opened)
+            os = null;
+            clientReader = null;
+            clientSocket = null;
+        }
+        }
+    }
+    
+    return terminatePerlScript; // if true, it will call destroy() on the SafeProcess' process
+    }
+
 
     /** Called by the WGet native code when the current download is 
@@ -542 +915 @@
      */
     public void downloadComplete() {
-    progress.downloadComplete();
+    progress.downloadComplete(); // now this is synchronized
     }
 
@@ -548 +921 @@
     public void downloadComplete(String current_file_downloading)
     {
-    progress.downloadComplete();
+    progress.downloadComplete(); // now this is synchronized
     DebugStream.println("Download complete: " + current_file_downloading);
     }
@@ -558 +931 @@
     public void downloadFailed() {
     // TODO!!
+    //synchronized(failed_urls) {
     //failed_urls.add(current_url); // It is the current url that failed
-    progress.downloadFailed();
+    //}
+    progress.downloadFailed(); // now this is synchronized
     //DebugStream.println("Download failed: " + current_url);
     } 
@@ -566 +941 @@
      */
     public void downloadWarning() {
-    progress.downloadWarning();
+    progress.downloadWarning(); // now this is synchronized
     }
 
@@ -584 +959 @@
      * @return An int representing the current DownloadJob state.
      */
-    public int getState() {
+    public synchronized int getState() {
     return state;
     }
@@ -592 +967 @@
      * stop.
      */
-    public boolean hasSignalledStop() {
+    public synchronized boolean hasSignalledStop() {
     if(state == DownloadJob.STOPPED || state == DownloadJob.PAUSED || 
        state == DownloadJob.COMPLETE) {
@@ -600 +975 @@
     }
 
-    public void setState(int state) {
+    public synchronized void setState(int state) {
     previous_state = this.state;
     this.state = state;
@@ -624 +999 @@
 
     
-    // Inner thread class that reads from process downloadfrom.pl's errorstream
-    private class PerlReaderThread extends Thread {
-    Process prcs = null;
-
-    public PerlReaderThread(Process proc) {
-        this.prcs = proc;
-    }
-
-    public void run() {
+    /*
+      Go through https://docs.oracle.com/javase/tutorial/essential/concurrency/atomicvars.html series of
+      Java articles on concurrency again.
+      Go through http://docs.oracle.com/javase/tutorial/uiswing/concurrency/
+
+      http://stackoverflow.com/questions/574240/is-there-an-advantage-to-use-a-synchronized-method-instead-of-a-synchronized-blo
+
+       "Not only do synchronized methods not lock the whole class, but they don't lock the whole instance either. Unsynchronized methods in the class may still proceed on the instance."
+       "Only the syncronized methods are locked. If there are fields you use within synced methods that are accessed by unsynced methods, you can run into race conditions."
+       
+       "synchronizing on "this" is considered in some circles to be an anti-pattern. The unintended consequence is that outside of the class someone can lock on an object reference that is equal to "this" and prevent other threads from passing the barriers within the class potentially creating a deadlock situation. Creating a "private final Object = new Object();" variable purely for locking purposes is the often used solution. Here's another question relating directly to this issue. http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java?lq=1"
+
+       "A private lock is a defensive mechanism, which is never a bad idea.
+
+       Also, as you alluded to, private locks can control granularity. One set of operations on an object might be totally unrelated to another but synchronized(this) will mutually exclude access to all of them."
+
+     http://stackoverflow.com/questions/8393883/is-synchronized-keyword-exception-safe
+     "In any scoped thread-safe block, the moment you get out of it, the thread-safety is gone."
+     "In case of an exception the lock will be released."
+
+     http://stackoverflow.com/questions/8259479/should-i-synchronize-listener-notifications-or-not
+     "Use a CopyOnWriteArrayList for your listener arrays."
+     "If you use the CopyOnWriteArrayList, then you don't have to synchronize when iterating."
+     "CopyOnWriteArrayList is thread-safe, so there is no need to synchronize."
+
+     "Use a ConcurrentLinkedQueue<Listener> ... for this kind of problems: adding, removing and iterating simultaneously on a collection.
+     A precision : this solution prevents a listener from being called from the very moment it is deregistered."
+     "It means that you start iterating, an element is added, it will be called, another is removed, it won't, all this in the same iteration cycle.
+     It's the best of both world: ensuring synchronization, while being fine grained on who gets called and who's not."
+
+     http://stackoverflow.com/questions/8260205/when-a-listener-is-removed-is-it-okay-that-the-event-be-called-on-that-listener
+
+     http://stackoverflow.com/questions/2282166/java-synchronizing-on-primitives
+     
+     1. You can't lock on a primitive and
+     2. Don't lock on a Long unless you're careful how you construct them. Long values created by autoboxing or Long.valueOf() in a certain range are guaranteed to be the same across the JVM which means other threads could be locking on the same exact Long object and giving you cross-talk. This can be a subtle concurrency bug (similar to locking on intern'ed strings).
+
+     Cross-talk:
+     "In electronics, crosstalk is any phenomenon by which a signal transmitted on one circuit or channel of a transmission system creates an undesired effect in another circuit or channel. Crosstalk is usually caused by undesired capacitive, inductive, or conductive coupling from one circuit, part of a circuit, or channel, to another."
+    */
+
+
+    // Inner thread class that reads from process downloadfrom.pl's std output stream
+    private class ProcessOutHandler extends SafeProcess.CustomProcessHandler {
+    
+    public ProcessOutHandler() { 
+        super(SafeProcess.STDOUT);
+    }
+
+    public void run(Closeable stream) {
+        InputStream is = (InputStream) stream;
+        BufferedReader eReader = null;
         try {
-        if(prcs != null) {
-            String message = null;
-            BufferedReader eReader = new BufferedReader(new InputStreamReader(prcs.getInputStream()));
-            while(prcs != null && (message = eReader.readLine()) != null) {
-            if(!message.equals("\n")) {
-                System.err.println("**** Perl STDOUT: " + message);
-            }
-            }
-            
-            if(prcs != null && eReader != null) {
-            eReader.close();
-            eReader = null;
-            System.err.println("**** Perl ENDed.");
-            }
-        }
+        
+        String message = null;
+        eReader = new BufferedReader(new InputStreamReader(is));
+        while(!Thread.currentThread().isInterrupted() && (message = eReader.readLine()) != null) {
+            if(!message.equals("\n")) {
+            System.err.println("**** Perl STDOUT: " + message);
+            }
+        }
+        if(Thread.currentThread().isInterrupted()) {
+            System.err.println("**** Perl INTERRUPTed.");
+        } else {
+            System.err.println("**** Perl ENDed.");
+        }
+        
         } catch(Exception e) {
         System.err.println("Thread - caught exception: " + e);
-        }
+        } finally {
+        if(Thread.currentThread().isInterrupted()) {
+            SafeProcess.log("@@@ Successfully interrupted " + Thread.currentThread().getName() + ".");
+        }
+        SafeProcess.closeResource(eReader);
+        eReader = null;
+        }
+    }
+    }
+
+
+    private class ProcessErrHandler extends SafeProcess.CustomProcessHandler {
+    
+    public ProcessErrHandler() { 
+        super(SafeProcess.STDERR);
+    }
+
+    public void run(Closeable stream) {
+        InputStream eis = (InputStream) stream;
+        
+        BufferedReader br = null;
+        try {
+        br = new BufferedReader(new InputStreamReader(eis));
+        
+        // Capture the standard error stream and search for two particular occurrences.
+        String line="";
+        boolean ignore_for_robots = false;
+        int max_download = DownloadJob.UNKNOWN_MAX;
+        
+        // handle to outer class objects that need synchronization (on either objects or their methods)
+        DownloadProgressBar progress = DownloadJob.this.progress;
+        AppendLineOnlyFileDocument download_log = DownloadJob.this.download_log;
+
+        while (!Thread.currentThread().isInterrupted() && (line = br.readLine()) != null
+               && !line.trim().equals("<<Finished>>") /*&& !isStopped()*/) {
+            if (max_download == DownloadJob.UNKNOWN_MAX) {
+            if(line.lastIndexOf("<<Defined Maximum>>") != -1) {
+                max_download = DownloadJob.DEFINED_MAX;
+            }
+            else if (line.lastIndexOf("<<Undefined Maximum>>") != -1) {
+                max_download = DownloadJob.UNDEFINED_MAX;
+            }
+            }
+            else if(max_download == DownloadJob.UNDEFINED_MAX) {
+            DebugStream.println(line);          
+            download_log.appendLine(line); // now synchronized
+            // The first magic special test is to see if we've just
+            // asked for the robots.txt file. If so we ignore
+            // the next add and then the next complete/error.
+            if(line.lastIndexOf("robots.txt;") != -1) {
+                DebugStream.println("***** Requesting robot.txt");
+                ignore_for_robots = true;
+            }
+            // If line contains "=> `" display text as the 
+            // currently downloading url. Unique to add download.
+            else if(line.lastIndexOf("=> `") != -1) {
+                if(!ignore_for_robots) {
+                // Add download
+                String new_url = line.substring(line.indexOf("`") + 1, line.lastIndexOf("'"));
+                
+                // now synchronized
+                progress.addDownload("file"); //addDownload("http:/" + new_url.substring(cachedir_prefix_length()-1));
+                }
+            }
+            // If line contains "/s) - `" set currently 
+            // downloading url to "Download Complete".
+            else if(line.lastIndexOf("/s) - `") != -1) {
+                String current_file_downloading = line.substring(line.indexOf("`") + 1, line.lastIndexOf("'"));
+                if(!ignore_for_robots) {
+                DebugStream.println("Not ignore for robots");
+                // Download complete
+                downloadComplete(current_file_downloading); // synchronized
+                }
+                else {
+                DebugStream.println("Ignore for robots");
+                ignore_for_robots = false;
+                }
+            }
+            // The already there line begins "File `..." However this
+            // is only true in english, so instead I looked and there
+            // are few (if any at all) other messages than those above
+            // and not overwriting messages that use " `" so we'll 
+            // look for that. Note this method is not guarenteed to be
+            // unique like the previous two.
+            else if(line.lastIndexOf(" `") != -1) {
+                // Not Overwriting
+                DebugStream.println("Already there.");
+                String new_url = line.substring(line.indexOf("`") + 1, line.lastIndexOf("'"));
+                
+                progress.addDownload("file"); //addDownload("http:/" + new_url.substring(cachedir_prefix_length()-1));
+                downloadWarning();
+            }
+            // Any other important message starts with the time in the form hh:mm:ss
+            else if(line.length() > 7) {
+                if(line.charAt(2) == ':' && line.charAt(5) == ':') {
+                if(!ignore_for_robots) {
+                    DebugStream.println("Error.");
+                    downloadFailed();
+                }
+                else {
+                    ignore_for_robots = false;
+                }
+                }
+            }
+            }
+            else if (max_download == DownloadJob.DEFINED_MAX) {
+            if (line.lastIndexOf("<<Total number of record(s):") != -1) {
+                String total_ID = line.substring(line.indexOf(":") + 1, line.indexOf(">"));
+
+                progress.setTotalDownload((Integer.valueOf(total_ID)).intValue());
+                progress.resetFileCount();  
+                progress.addDownload("files"); // for display: "Downloading files"
+
+            }
+            else if (line.lastIndexOf("<<Done>>") != -1) {
+                progress.increaseFileCount();
+            }
+            else if(line.lastIndexOf("<<Done:") != -1) {
+                String completed_amount = line.substring(line.indexOf(":") + 1, line.indexOf(">"));
+                progress.increaseFileCount((Integer.valueOf(completed_amount)).intValue());
+            }
+            
+            DebugStream.println(line);
+            download_log.appendLine(line);
+            }
+            else {
+            System.out.println("Error!!");
+            System.exit(-1);
+            }
+        }
+        
+        } catch (IOException ioe) {
+        //message(Utility.ERROR, ioe.toString());
+        //JTest
+        DebugStream.printStackTrace(ioe);
+
+        } finally {
+        if(Thread.currentThread().isInterrupted()) { // if the thread this class is running in is interrupted
+            SafeProcess.log("@@@ Successfully interrupted " + Thread.currentThread().getName() + ".");          
+        }
+
+        SafeProcess.closeResource(br);
+        br = null;
+        }
+        
     }
     }

main/trunk/gli/src/org/greenstone/gatherer/download/DownloadProgressBar.java

@@ -128 +128 @@
     JPanel button_pane = new JPanel();
 
+    // our "pause" button never paused before, it always did a process.destroy() on being pressed.
+    // See http://trac.greenstone.org/browser/trunk/gli/src/org/greenstone/gatherer/download/DownloadJob.java?rev=13594
+    // However, now we additionally ensure that the wget launched by the perl is stopped before 
+    // process.destroy(), so at least it cleans up better. I'm therefore changing it to a "Stop" button.
     stop_start_button = new GLIButton(Dictionary.get("Mirroring.DownloadJob.Pause"),Dictionary.get("Mirroring.DownloadJob.Pause_Tooltip"));
+    //stop_start_button = new GLIButton(Dictionary.get("Mirroring.DownloadJob.Stop"),Dictionary.get("Mirroring.DownloadJob.Stop_Tooltip"));
     stop_start_button.addActionListener(this);
     stop_start_button.addActionListener(owner);
@@ -166 +171 @@
     // Make the labels, etc update.
     refresh();
+    }
+
+    // internally thread-safe
+    public void enableCancelJob(boolean isEnabled) {
+    synchronized(stop_start_button) {
+        stop_start_button.setEnabled(isEnabled);
+    }
+    synchronized(stop_start_button) {
+        close_button.setEnabled(isEnabled);
+    }
     }
 
@@ -212 +227 @@
      * @param url The url String of the file that is being downloaded.
      */
-    public void addDownload(String url) {
+    public synchronized void addDownload(String url) {
     current_url = url;
     file_size = 0;
@@ -221 +236 @@
      * is called to enlighten the DownloadProgressBar of this fact.
      */
-    public void downloadComplete() {
+    public synchronized void downloadComplete() {
     current_url = null;
     file_count++;
@@ -232 +247 @@
     }
 
-    public void downloadFailed() {
+    public synchronized void downloadFailed() {
     err_count++;
     if(total_count < (file_count + err_count + warning_count)) {
@@ -240 +255 @@
     }
 
-    public void downloadWarning() {
+    public synchronized void downloadWarning() {
     warning_count++;
     if(total_count < (file_count + err_count + warning_count)) {
@@ -248 +263 @@
     }
 
-    public void setTotalDownload(int total_download) {
+    // need to make these methods synchronized too, as they modify variables
+    // that other synchronized methods work with. And if any methods that modify
+    // such variables were to remain unsynchronized, can end up with race conditions
+    // http://stackoverflow.com/questions/574240/is-there-an-advantage-to-use-a-synchronized-method-instead-of-a-synchronized-blo
+    // "Not only do synchronized methods not lock the whole class, but they don't lock the whole instance either. Unsynchronized methods in the class may still proceed on the instance."
+    // "Only the syncronized methods are locked. If there are fields you use within synced methods that are accessed by unsynced methods, you can run into race conditions."
+    public synchronized void setTotalDownload(int total_download) {
     total_count = total_download;
     refresh();
@@ -257 +278 @@
     }
 
-    public void increaseFileCount() {
+    public synchronized void increaseFileCount() {
     file_count++;
     refresh();
     }
 
-    public void increaseFileCount(int amount) {
+    public synchronized void increaseFileCount(int amount) {
     file_count += amount;
     refresh();
     }
 
-    public void resetFileCount() {
+    public synchronized void resetFileCount() {
     file_count = 0;
     refresh();
@@ -278 +299 @@
      * reset to zero.
      */
-    public void mirrorBegun(boolean reset, boolean simple) {
+    public synchronized void mirrorBegun(boolean reset, boolean simple) {
     if(reset) {
         this.file_count = 0;
@@ -301 +322 @@
      * components.
      */
-    public void mirrorComplete() {
+    public synchronized void mirrorComplete() {
     current_action = DownloadJob.COMPLETE;
     current_url = null;
@@ -319 +340 @@
      * the amount of the current file downloaded.
      */
-    public void updateProgress(long current, long expected) {
+    public synchronized void updateProgress(long current, long expected) {
     file_size = file_size + current;
     if(!progress.isIndeterminate()) {

main/trunk/gli/src/org/greenstone/gatherer/download/DownloadScrollPane.java

@@ -44 +44 @@
 import javax.swing.tree.*;
 import org.greenstone.gatherer.*;
+import org.greenstone.gatherer.util.SafeProcess;
 
 /** This class provides access to the functionality of the WGet program, either by calling it via a shell script or by the JNI. It maintains a queue of pending jobs, and the component for showing these tasks to the user.
@@ -78 +79 @@
     }
 
-    public void deleteDownloadJob(DownloadJob delete_me) {
+    /** 
+     * To be used with DownloadJob.java's old_callDownload() and old_actionPerformed()
+     * OR by uncommenting the "synchronized(this)" section in Download.java at the end of
+     * its new callDownload() along with commenting out "mummy.deleteCurrentDownloadJob(this);"
+     * in Download.java's doneCleanup().
+    */
+    public void old_deleteDownloadJob(DownloadJob delete_me) {
     if (delete_me == job) {
         try {
@@ -86 +93 @@
         synchronized(delete_me) {
             if (!delete_me.hasSignalledStop()) { // don't wait if DownloadJob.COMPLETED 
+            ///SafeProcess.log("**************** download scrollpane waiting for downloadjob to stop");
             delete_me.wait();
             }
@@ -93 +101 @@
         }       
     }
+
+    ///System.err.println("**************** Deleting job from download scroll pane");
     // Close button pressed, get rid of this download's dedicated pane
     finishedDownloadJob(delete_me, true);
     }
+
+    /**
+     * If called to delete the current download job, this method won't do anything.
+     * But if called on any inactive download job, its display is removed.
+    */
+    public void deleteDownloadJob(DownloadJob delete_me) {
+    if (delete_me != job) {
+
+        SafeProcess.log("**************** Deleting job from download scroll pane");
+        // Close button pressed, get rid of this download's dedicated pane
+        finishedDownloadJob(delete_me, true);
+    } // else don't do anything, we'll be contacted again when the current job can be deleted
+
+    }
+
+    /**
+     * To be called when we're ready to delete the current download job,
+     * else this method won't do anything
+    */
+    public void deleteCurrentDownloadJob(DownloadJob delete_me) {
+    if (delete_me == job) {
+        SafeProcess.log("**************** Deleting current job from download scroll pane");
+        // Close button pressed, get rid of this download's dedicated pane
+        finishedDownloadJob(delete_me, true);
+    }
+    }
+
 
     /** To be called when a download job has terminated naturally or was prematurely stopped

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

@@ -63 +63 @@
 
 /** The <strong>GShell</strong> is reponsible for running a separately threaded process in the command shell. This is necessary for executing the Perl Scripts and also for other system related funcitonality.
+ * When modifying this class, bear in mind concurrency issues that could arise with SafeProcess'
+ * worker threads and where synchronization may be needed to prevent such issues.
  */
 public class GShell
@@ -390 +392 @@
         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 to any worker threads launched by the SafeProcess
+        // The following is implemented to interrupt the SafeProcess thread (which is *this* GShell thread),
+        // but only if any process is being run by SafeProcess.
+        // This will propagate the CANCEL status to any worker threads launched by the SafeProcess.
+        // Everything will clean up nicely after itself (even terminating subprocesses that prcs launched on Windows!)
+        prcs.cancelRunningProcess(); 
+        
         prcs = null;        
     }

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

@@ -10 +10 @@
 import java.io.OutputStream;
 import java.io.OutputStreamWriter;
+import java.net.Socket;
 import java.util.Arrays;
 import java.util.Scanner;
 import java.util.Stack;
+import javax.swing.SwingUtilities;
+
 
 import com.sun.jna.*;
@@ -39 +42 @@
     public static final int STDIN = 2;
     // can't make this variable final and init in a static block, because it needs to use other SafeProcess static methods which rely on this in turn:
-    public static String WIN_KILL_CMD; 
-        
+    public static String WIN_KILL_CMD;
+
+    /**
+     * Boolean interruptible is used to mark any sections of blocking code that should not be interrupted
+     * with an InterruptedExceptions. At present only the cancelRunningProcess() attempts to do such a thing
+     * and avoids doing so when interruptible is false.
+     * Note that interruptible is also used as a lock, so remember to synchronize on it when using it!
+    */
+    public Boolean interruptible = Boolean.TRUE; 
     
     // charset for reading process stderr and stdout streams
@@ -55 +65 @@
     private Process process = null;
     private boolean forciblyTerminateProcess = false;
+
+    /** a ref to the thread in which the Process is being executed (the thread wherein Runtime.exec() is called) */
+    private Thread theProcessThread = null;
     
     // output from running SafeProcess.runProcess()
@@ -64 +77 @@
     // allow callers to process exceptions of the main process thread if they want
     private ExceptionHandler exceptionHandler = null;
+    /** allow callers to implement hooks that get called during the main phases of the internal
+     * process' life cycle, such as before and after process.destroy() gets called
+    */
+    private MainProcessHandler mainHandler = null;
 
     // whether std/err output should be split at new lines
@@ -110 +127 @@
     }
 
+    /** to set a handler that will handle the main (SafeProcess) thread,
+     * implementing the hooks that will get called during the internal process' life cycle,
+     * such as before and after process.destroy() is called */
+    public void setMainHandler(MainProcessHandler handler) {
+    this.mainHandler = 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) {
@@ -118 +142 @@
     }
 
+    
+    /*
+    public boolean canInterrupt() {
+    boolean canInterrupt;
+    synchronized(interruptible) {
+        canInterrupt = interruptible.booleanValue();
+    }
+    return canInterrupt;
+    }
+    */
+
+    /**
+     * Call this method when you want to prematurely and safely terminate any process
+     * that SafeProcess may be running.
+     * You may want to implement the SafeProcess.MainHandler interface to write code
+     * for any hooks that will get called during the process' life cycle.
+     * @return false if process has already terminated or if it was already terminating
+     * when cancel was called. In such cases no interrupt is sent. Returns boolean sentInterrupt.
+     */
+    public synchronized boolean cancelRunningProcess() {
+    // on interrupt:
+    // - forciblyTerminate will be changed to true if the interrupt came in when the process was
+    // still running (and so before the process' streams were being joined)
+    // - and forciblyTerminate will still remain false if the interrupt happens when the process'
+    // streams are being/about to be joined (hence after the process naturally terminated).
+    // So we don't touch the value of this.forciblyTerminate here.
+    // The value of forciblyTerminate determines whether Process.destroy() and its associated before
+    // and after handlers are called or not: we don't bother destroying the process if it came to
+    // a natural end.
+
+    // no process to interrupt, so we're done
+    if(this.process == null) {
+        log("@@@ No Java process to interrupt.");
+        return false;
+    }
+    
+    boolean sentInterrupt = false;
+
+    // can't interrupt when SafeProcess is joining (cleanly terminating) worker threads
+    // have to wait until afterward     
+    if (interruptible) {
+        // either way, we can now interrupt the thread - if we have one (we should)
+        if(this.theProcessThread != null) { // we're told which thread should be interrupted
+        this.theProcessThread.interrupt();
+        log("@@@ Successfully sent interrupt to process.");
+        sentInterrupt = true;
+        }
+    }
+    else { // wait for join()s to finish.
+        // During and after joining(), there's no need to interrupt any more anyway: no calls
+        // subsequent to joins() block, so everything thereafter is insensitive to InterruptedExceptions.
+
+        if(SwingUtilities.isEventDispatchThread()) {
+        log("#### Event Dispatch thread, returning");
+        return false;
+        }
+
+        while(!interruptible) {
+
+        log("######### Waiting for process to become interruptible...");
+        
+        // https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html
+        // wait will release lock on this object, and regain it when loop condition interruptible is true
+        try {
+            this.wait(); // can't interrupt when SafeProcess is joining (cleanly terminating) worker threads, so wait
+        } catch(Exception e) {
+            log("@@@ Interrupted exception while waiting for SafeProcess' worker threads to finish joining on cancelling process");
+        }
+        }
+
+        // now the process is sure to have ended as the worker threads would have been joined
+    }
+
+    return sentInterrupt;
+    }
+
+    
     // In future, think of changing the method doRuntimeExec() over to using ProcessBuilder
     // instead of Runtime.exec(). ProcessBuilder seems to have been introduced from Java 5.
     // https://docs.oracle.com/javase/7/docs/api/java/lang/ProcessBuilder.html
-    // https://zeroturnaround.com/rebellabs/how-to-deal-with-subprocesses-in-java/
+    // See also https://zeroturnaround.com/rebellabs/how-to-deal-with-subprocesses-in-java/
     // which suggests using Apache Common Exec to launch processes and says what will be forthcoming in Java 9
     
@@ -159 +260 @@
     }
 
+    this.theProcessThread = Thread.currentThread(); // store a ref to the thread wherein the Process is being run
     return prcs;
     }
@@ -173 +275 @@
     outputGobbler.start();
     
-    // any error???
     try {
-        this.exitValue = process.waitFor(); // can throw an InterruptedException if process did not terminate
+        this.exitValue = process.waitFor(); // can throw an InterruptedException if process was cancelled/prematurely terminated
     } catch(InterruptedException ie) {
         log("*** Process interrupted (InterruptedException). Expected to be a Cancel operation.");
@@ -185 +286 @@
         // 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.
+        // which will be caught by the calling method's own catch on InterruptedException.
         // Only if the thread that SafeProcess runs in was interrupted
         // should we propagate the interrupt to the worker threads.
@@ -215 +316 @@
         
         //log("Process exitValue: " + exitValue);
+        ///log("@@@@ Before join phase. Forcibly terminating: " + this.forciblyTerminateProcess);
         
         // From the comments of 
@@ -225 +327 @@
         // 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(); 
+
+
+        // Thread.joins() below are blocking calls, same as Process.waitFor(), and a cancel action could
+        // send an interrupt during any Join: the InterruptedException ensuing will then break out of the
+        // joins() section. We don't want that to happen: by the time the joins() start happening, the
+        // actual process has finished in some way (naturally terminated or interrupted), and nothing
+        // should interrupt the joins() (nor ideally any potential p.destroy after that).
+        // So we mark the join() section as an un-interruptible section, and make anyone who's seeking
+        // to interrupt just then first wait for this Thread (in which SafeProcess runs) to become
+        // interruptible again. Thos actually assumes anything interruptible can still happen thereafter
+        // when in reality, none of the subsequent actions after the joins() block. So they nothing
+        // thereafter, which is the cleanup phase, will actually respond to an InterruptedException.
+
+        
+        if(this.mainHandler != null) {
+        // this method can unset forcible termination flag
+        // if the process had already naturally terminated by this stage:
+        this.forciblyTerminateProcess = mainHandler.beforeWaitingForStreamsToEnd(this.forciblyTerminateProcess);
+        }
+
+        ///log("@@@@ After beforeJoin Handler. Forcibly terminating: " + this.forciblyTerminateProcess);
+
+        // Anyone could interrupt/cancel during waitFor() above,
+        // but no one should interrupt while the worker threads come to a clean close,
+        // so make anyone wanting to cancel the process at this stage wait()
+        // until we're done with the join()s:
+        synchronized(interruptible) {
+        interruptible = Boolean.FALSE;
+        }
+        //Thread.sleep(5000); // Uncomment to test this uninterruptible section, also comment out block checking for
+            // EventDispatchThread in cancelRunningProcess() and 2 calls to progress.enableCancelJob() in DownloadJob.java
+        outputGobbler.join();
         errorGobbler.join();
-        inputGobbler.join();
-        
-        
+        inputGobbler.join(); 
+
+        synchronized(interruptible) {
+        interruptible = Boolean.TRUE;
+        }
+
+        ///log("@@@@ Join phase done...");
+
+        // notify any of those waiting to interrupt this thread, that they may feel free to do so again
+        // https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html
+        synchronized(this) {
+        this.notify();
+        }
+
         // set the variables that the code which created a SafeProcess object may want to inspect
         this.outputStr = outputGobbler.getOutput();
-        this.errorStr = errorGobbler.getOutput();       
+        this.errorStr = errorGobbler.getOutput();
+        
+        // call the after join()s hook
+        if(this.mainHandler != null) {
+        this.forciblyTerminateProcess = mainHandler.afterStreamsEnded(this.forciblyTerminateProcess);
+        }
     }
 
@@ -283 +428 @@
         
         Thread.currentThread().interrupt();
-    } finally { 
-
-        if( this.forciblyTerminateProcess ) {
-        destroyProcess(process); // see runProcess() below      
-        }
-        process = null;
-        this.forciblyTerminateProcess = false; // reset
+    } finally {
+        
+        cleanUp("SafeProcess.runBasicProcess");
     }
     return this.exitValue;
@@ -347 +488 @@
 
 
-            // 3. kick off the stream gobblers
+        // 3. kick off the stream gobblers
         this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
        
@@ -365 +506 @@
         log("@@@@ Unexpected InterruptedException when waiting for process stream gobblers to die");
         } else {
-        log("*** Unexpected InterruptException when waiting for process stream gobblers to die:" + ie.getMessage(), ie);
+        log("*** Unexpected InterruptException when waiting for process stream gobblers to die: " + ie.getMessage(), ie);
         }
 
@@ -371 +512 @@
         Thread.currentThread().interrupt();
     
-    } finally { 
-        //String cmd = (this.command == null) ? Arrays.toString(this.command_args) : this.command;
-        //log("*** In finally of SafeProcess.runProcess(3 params): " + cmd);
-
-        if( this.forciblyTerminateProcess ) {
-        log("*** Going to call process.destroy 2");
-        destroyProcess(process);                
-        log("*** Have called process.destroy 2");
-        }
-        process = null;
-        this.forciblyTerminateProcess = false; // reset
+    } finally {
+        
+        cleanUp("SafeProcess.runProcess(3 params)");
     }
     
@@ -424 +557 @@
 
 
-        // 4. kick off the stream gobblers
+        // 3. kick off the stream gobblers
         this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
        
@@ -452 +585 @@
         Thread.currentThread().interrupt(); // re-interrupt the thread - which thread? Infinite loop?
     
-    } finally { 
-
-        // 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        
-        
-        //String cmd = (this.command == null) ? Arrays.toString(this.command_args) : this.command;
-        //log("*** In finally of SafeProcess.runProcess(2 params): " + cmd);
-
-        if( this.forciblyTerminateProcess ) {
-        log("*** Going to call process.destroy 1");
-        destroyProcess(process);    
-        log("*** Have called process.destroy 1");
-        }
-        process = null;
-        this.forciblyTerminateProcess = false; //reset      
+    } finally {
+        
+        cleanUp("SafeProcess.runProcess(2 params)");    
     }
     
     return this.exitValue;
+    }
+
+    private void cleanUp(String callingMethod) {
+    
+    // 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        
+
+    //String cmd = (this.command == null) ? Arrays.toString(this.command_args) : this.command;
+    //log("*** In finally of " + callingMethod + ": " + cmd);
+
+    // if we're forcibly terminating the process, call the before- and afterDestroy hooks
+    // besides actually destroying the process
+    if( this.forciblyTerminateProcess ) {
+        log("*** Going to call process.destroy from " + callingMethod);
+
+        if(mainHandler != null) mainHandler.beforeProcessDestroy();
+        SafeProcess.destroyProcess(process); // see runProcess(2 args/3 args)
+        if(mainHandler != null) mainHandler.afterProcessDestroy();
+
+        log("*** Have called process.destroy from " + callingMethod);
+    }   
+
+    process = null;
+    this.theProcessThread = null; // let the process thread ref go too
+    boolean wasForciblyTerminated = this.forciblyTerminateProcess;
+    this.forciblyTerminateProcess = false; // reset
+    
+    if(mainHandler != null) mainHandler.doneCleanup(wasForciblyTerminated);
     }
 
@@ -608 +757 @@
 // e.g. full-import.pl may be terminated with p.destroy(), but it launches import.pl which is left running until it naturally terminates.
 static void destroyProcess(Process p) {
+    log("### in SafeProcess.destroyProcess(Process p)");
+
     // If it isn't windows, process.destroy() terminates any child processes too
     if(!Utility.isWindows()) {
@@ -626 +777 @@
     // so we can use it to find the pids of any subprocesses it launched in order to terminate those too.
     
-    long processID = SafeProcess.getProcessID(p);       
-    log("Attempting to terminate sub processes of Windows process with pid " + processID);
-    terminateSubProcessesRecursively(processID, p);
+    long processID = SafeProcess.getProcessID(p);
+    if(processID == -1) { // the process doesn't exist or no longer exists (terminated naturally?)
+    p.destroy(); // minimum step, do this anyway, at worst there's no process and this won't have any effect
+    } else {
+    log("Attempting to terminate sub processes of Windows process with pid " + processID);
+    terminateSubProcessesRecursively(processID, p);
+    }
     
 }
@@ -706 +861 @@
 private static String getWinProcessKillCmd(Long processID) {
     // check if we first need to init WIN_KILL_CMD. We do this only once, but can't do it in a static codeblock
-    
+    // because of a cyclical dependency regarding this during static initialization
+
     if(WIN_KILL_CMD == null) {      
     if(SafeProcess.isAvailable("wmic")) {
@@ -734 +890 @@
 // where is not part of winbin. where is a system command on windows, but only since 2003, https://ss64.com/nt/where.html
 // There is no `where` on Linux/Mac, must use which for them.
-// On windows, "which tskill" fails but "which" succeeds on taskkill|wmic|browser names.
+// On windows, "which tskill" fails (and "where tskill" works), but "which" succeeds on taskkill|wmic|browser names.
 public static boolean isAvailable(String program) {     
     try {
@@ -764 +920 @@
 public static interface ExceptionHandler {
 
-    // when implementing ExceptionHandler.gotException(), if it manipulates anything that's
-    // not threadsafe, declare gotException() as a synchronized method to ensure thread safety
-    public void gotException(Exception e); // can't declare as synchronized in interface method declaration
+    /**
+     * Called whenever an exception occurs during the execution of the main thread of SafeProcess
+     * (the thread in which the Process is run).
+     * Since this method can't be declared as synchronized in this interface method declaration,
+     * when implementing ExceptionHandler.gotException(), if it manipulates anything that's
+     * not threadsafe, declare gotException() as a synchronized method to ensure thread safety
+     */
+    public void gotException(Exception e);
+}
+
+/** On interrupting (cancelling) a process, 
+ * if the class that uses SafeProcess wants to do special handling 
+ * either before and after join() is called on all the worker threads,
+ * or, only on forcible termination, before and after process.destroy() is to be called,
+ * then that class can implement this MainProcessHandler interface
+ */
+public static interface MainProcessHandler {
+    /** 
+     * Called before the streamgobbler join()s.
+     * If not overriding, the default implementation should be:
+     * public boolean beforeWaitingForStreamsToEnd(boolean forciblyTerminating) { return forciblyTerminating; }
+     * When overriding:
+     * @param forciblyTerminating is true if currently it's been decided that the process needs to be
+     * forcibly terminated. Return false if you don't want it to be. For a basic implementation,
+     * return the parameter.
+     * @return true if the process is still running and therefore still needs to be destroyed, or if
+     * you can't determine whether it's still running or not. Process.destroy() will then be called.
+     * @return false if the process has already naturally terminated by this stage. Process.destroy()
+     * won't be called, and neither will the before- and after- processDestroy methods of this class.
+    */
+    public boolean beforeWaitingForStreamsToEnd(boolean forciblyTerminating);
+    /** 
+     * Called after the streamgobbler join()s have finished.
+     * If not overriding, the default implementation should be:
+     * public boolean afterStreamsEnded(boolean forciblyTerminating) { return forciblyTerminating; }
+     * When overriding:
+     * @param forciblyTerminating is true if currently it's been decided that the process needs to be
+     * forcibly terminated. Return false if you don't want it to be. For a basic implementation,
+     * return the parameter (usual case).
+     * @return true if the process is still running and therefore still needs to be destroyed, or if
+     * can't determine whether it's still running or not. Process.destroy() will then be called.
+     * @return false if the process has already naturally terminated by this stage. Process.destroy()
+     * won't be called, and neither will the before- and after- processDestroy methods of this class.
+    */
+    public boolean afterStreamsEnded(boolean forciblyTerminating);
+    /**
+     * called after join()s and before process.destroy()/destroyProcess(Process), iff forciblyTerminating
+     */
+    public void beforeProcessDestroy(); 
+    /**
+     * Called after process.destroy()/destroyProcess(Process), iff forciblyTerminating
+     */
+    public void afterProcessDestroy();
+
+    /** 
+     * Always called after process ended: whether it got destroyed or not
+     */
+    public void doneCleanup(boolean wasForciblyTerminated);
 }
 
@@ -1080 +1291 @@
     }
 
+    // in Java 6, Sockets don't yet implement Closeable
+    public static boolean closeSocket(Socket resourceHandle) {
+    boolean success = false;
+    try {
+        if(resourceHandle != null) {
+        resourceHandle.close();
+        resourceHandle = null;
+        success = true;
+        }
+    } catch(Exception e) {
+        log("Exception closing resource: " + e.getMessage(), e);
+        resourceHandle = null;
+        success = false;
+    } finally {
+        return success;
+    }
+    }
+
     public static boolean closeProcess(Process prcs) {
     boolean success = true;