Changeset 31703

Timestamp:

2017-05-25T18:47:14+12:00 (7 years ago)

Author:

ak19

Message:

When an instance of SafeProcess calls its static destroyProcess method, this happens on cancel action which would have caused an Interrupt to the SafeProcess thread that interrupts the worker threads, all of which cleanly tidy up after themselves. On Linux, this has the effect that the Process internal to SafeProcess, and any subprocesses that the process may have launched, are all neatly terminated. So on Linux, when destroyProcess is called by a SafeProcess instance on itself, there's no need to send a SIGTERM (else SIGKILL) to terminate the Process, as it's already cleanly terminated. Reducing the overkill step in such a scenario. The public static SafeProcess.destroyProcess(Process p) now forces the step of sending a termination signal, since any caller outside a SafeProcess instance won't neatly tidy up their Process object with interrupts.

File:

• main/trunk/gli/src/org/greenstone/gatherer/util/SafeProcess.java (modified) (5 diffs)

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

@@ -609 +609 @@
 
         if(mainHandler != null) mainHandler.beforeProcessDestroy();
-        SafeProcess.destroyProcess(process); // see runProcess(2 args/3 args)
+        boolean noNeedToDestroyIfOnLinux = true; // Interrupt handling suffices to cleanup process and subprocesses on Linux
+        SafeProcess.destroyProcess(process, noNeedToDestroyIfOnLinux); // see runProcess(2 args/3 args)
         if(mainHandler != null) mainHandler.afterProcessDestroy();
 
@@ -809 +810 @@
     // the process and its subprocesses (don't need to call this method at all to terminate the processes: the processes
     // aren't running when we get to this method)
-    log("@@@ Termination signal returned exitvalue 1, on linux this happens when the process has already been terminated");
+    log("@@@ Sending termination signal returned exit value 1. On linux this happens when the process has already been terminated");
     return true;
     } else {
@@ -820 +821 @@
 }
 
+public static void destroyProcess(Process p) {
+    // A cancel action results in an interruption to the process thread, which in turn interrupts
+    // the SafeProcess' worker threads, all which clean up after themselves.
+    // On linux, this suffices to cleanly terminate a Process and any subprocesses that may have launched
+    // so we don't need to do extra work in such a case. But the interrupts happen only when SafeProcess calls
+    // destroyProcess() on the Process it was running internally, and not if anyone else tries to end a 
+    // Process by calling SafeProcess.destroyProcess(p). In such cases, the Process needs to be actively terminated:
+    boolean canSkipExtraWorkIfLinux = true;
+    SafeProcess.destroyProcess(p, !canSkipExtraWorkIfLinux);
+}
 
 // On linux and mac, p.destroy() suffices to kill processes launched by p as well.
 // On Windows we need to do more work, since otherwise processes launched by p remain around executing until they naturally terminate.
 // e.g. full-import.pl may be terminated with p.destroy(), but it launches import.pl which is left running until it naturally terminates.
-public static void destroyProcess(Process p) {
+private static void destroyProcess(Process p, boolean canSkipExtraWorkIfLinux) {
     log("### in SafeProcess.destroyProcess(Process p)");
 
     // If it isn't windows, process.destroy() terminates any child processes too
-    if(!Utility.isWindows()) {
+    if(Utility.isWindows()) {
+    
+    if(!SafeProcess.isAvailable("wmic")) {
+        log("wmic, used to kill subprocesses, is not available. Unable to terminate subprocesses...");
+        log("Kill them manually from the TaskManager or they will proceed to run to termination");
+        
+        // At least we can get rid of the top level process we launched
+        p.destroy();
+        return;
+    }   
+    
+    // get the process id of the process we launched,
+    // 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);
+    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);
+    }
+    return;
+    
+    }
+    else { // linux or mac
+
+    // if we're on linux and would have already terminated by now (in which case canSkipExtraWorkForLinux would be true),
+    // then there's nothing much left to do. This would only be the case if SafeProcess is calling this method on its
+    // internal process, since it would have successfully cleaned up on Interruption and there would be no process left running
+    if(!Utility.isMac() && canSkipExtraWorkIfLinux) {
+        log("@@@ Linux: Cancelling a SafeProcess instance does not require any complicated system destroy operation");
+        p.destroy(); // vestigial: this will have no effect if the process had already terminated, which is the case in this block
+        return;
+    }
+    // else we're on a Mac or an external caller (not SafeProcess) has requested explicit termination on Linux
+    
     long pid = SafeProcess.getProcessID(p);
     /*
@@ -845 +891 @@
         }       
     }
-
+    
     return;
     }
@@ -851 +897 @@
     // else we're on windows:
 
-    if(!SafeProcess.isAvailable("wmic")) {
-    log("wmic, used to kill subprocesses, is not available. Unable to terminate subprocesses...");
-    log("Kill them manually from the TaskManager or they will proceed to run to termination");
-    
-    // At least we can get rid of the top level process we launched
-    p.destroy();
-    return;
-    }   
-    
-    // get the process id of the process we launched,
-    // 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);
-    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);
-    }
     
 }