Index: main/trunk/gli/src/org/greenstone/gatherer/util/SafeProcess.java
===================================================================
--- main/trunk/gli/src/org/greenstone/gatherer/util/SafeProcess.java	(revision 31649)
+++ main/trunk/gli/src/org/greenstone/gatherer/util/SafeProcess.java	(revision 31650)
@@ -12,4 +12,5 @@
 import java.util.Arrays;
 import java.util.Scanner;
+import java.util.Stack;
 
 import com.sun.jna.*;
@@ -28,11 +29,17 @@
 // to avoid blocking problems that can arise from a Process' input and output streams.
 
+// On Windows, Perl could launch processes as proper ProcessTrees: http://search.cpan.org/~gsar/libwin32-0.191/
+// Then killing the root process will kill child processes naturally.
+
 public class SafeProcess {
-    //public static int DEBUG = 0;
+    public static int DEBUG = 0;
 
     public static final int STDERR = 0;
     public static final int STDOUT = 1;
     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; 
+		
+	
     // charset for reading process stderr and stdout streams
     //public static final String UTF8 = "UTF-8";    
@@ -111,4 +118,9 @@
     }
 
+	// 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/
+	// which suggests using Apache Common Exec to launch processes and says what will be forthcoming in Java 9
     private Process doRuntimeExec() throws IOException {
 	Process prcs = null;
@@ -122,5 +134,12 @@
 	    
 	    // http://stackoverflow.com/questions/5283444/convert-array-of-strings-into-a-string-in-java
-	    log("SafeProcess running: " + Arrays.toString(command_args));
+		//log("SafeProcess running:" + Arrays.toString(command_args));
+		StringBuffer cmdDisplay = new StringBuffer();
+		for(int i = 0; i < command_args.length; i++) {
+			cmdDisplay.append(" ").append(command_args[i]);
+		}
+		log("SafeProcess running: [" + cmdDisplay + "]");
+		cmdDisplay = null; // let the GC have it    
+		
 	    
 	    if(this.envp == null) { 
@@ -231,5 +250,10 @@
 
     // Run a very basic process: with no reading from or writing to the Process' iostreams,
-    // this just execs the process and waits for it to return
+    // this just execs the process and waits for it to return.
+	// Don't call this method but the zero-argument runProcess() instead if your process will
+	// output stuff to its stderr and stdout streams but you don't need to monitory these.
+	// Because, as per a comment in GLI's GS3ServerThread.java,
+	// in Java 6, it wil block if you don't handle a process' streams when the process is
+	// outputting something. (Java 7+ won't block if you don't bother to handle the output streams)
     public int runBasicProcess() {
 	try {
@@ -399,5 +423,5 @@
 
 
-            // 4. kick off the stream gobblers
+	    // 4. kick off the stream gobblers
 	    this.exitValue = waitForWithStreams(inputGobbler, outputGobbler, errorGobbler);
        
@@ -450,30 +474,78 @@
     }
 
-// stackoverflow.com/questions/1835885/send-ctrl-c-to-process-open-by-java	
-// (Taskkill command can kill all running perl. But what if we kill perl instances not launched by GS?)
-// stackoverflow.com/questions/912889/how-to-send-interrupt-key-sequence-to-a-java-process
-// Searching for: "forcibly terminate external process launched by Java on Windows"	
-static void killAnyPerl() {
-	if(!Utility.isWindows()) return;
-	
-	// only on Windows do we need to still kill perl despite process.destroy()
-	try{
-		//Runtime.getRuntime().exec("taskkill /f /im perl.exe");
-		SafeProcess proc = new SafeProcess("taskkill /f /im perl.exe");
-			// /f Specifies to forcefully terminate the process(es).
-			//  /t Terminates the specified process and any child processes which were started by it.
-			// /im is the image name (the name of the program), see Image Name column in Win Task Manager.
-		int exitValue = proc.runBasicProcess(); // don't care about IOstreams
-	} catch(Exception e) {
-		log("@@@ Exception attempting to stop perl " + e.getMessage(), e);		
-	}
-}
+/*
+
+ On Windows, p.destroy() terminates process p that Java launched,
+ but does not terminate any processes that p may have launched. Presumably since they didn't form a proper process tree.
+    https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/e3cb7532-87f6-4ae3-9d80-a3afc8b9d437/how-to-kill-a-process-tree-in-cc-on-windows-platform?forum=vclanguage
+    https://msdn.microsoft.com/en-us/library/windows/desktop/ms684161(v=vs.85).aspx
+
+ Searching for: "forcibly terminate external process launched by Java on Windows"	
+ Not possible: stackoverflow.com/questions/1835885/send-ctrl-c-to-process-open-by-java 
+ But can use taskkill or tskill or wmic commands to terminate a process by processID
+ stackoverflow.com/questions/912889/how-to-send-interrupt-key-sequence-to-a-java-process
+ Taskkill command can kill by Image Name, such as all running perl, e.g. taskkill /f /im perl.exe
+ But what if we kill perl instances not launched by GS?
+    /f Specifies to forcefully terminate the process(es). We need this flag switched on to kill childprocesses.
+    /t Terminates the specified process and any child processes which were started by it. 
+			/t didn't work to terminate subprocesses. Maybe since the process wasn't launched as 
+			a properly constructed processtree.
+    /im is the image name (the name of the program), see Image Name column in Win Task Manager.
+	
+ We don't want to kill all perl running processes. 
+ Another option is to use wmic, available since Windows XP, to kill a process based on its command
+ which we sort of know (SafeProcess.command) and which can be seen in TaskManager under the 
+ "Command Line" column of the Processes tab.
+    https://superuser.com/questions/52159/kill-a-process-with-a-specific-command-line-from-command-line	
+ The following works kill any Command Line that matches -site localsite lucene-jdbm-demo
+    C:>wmic PATH win32_process Where "CommandLine like '%-site%localsite%%lucene-jdbm-demo%'" Call Terminate
+ "WMIC Wildcard Search using 'like' and %"
+    https://codeslammer.wordpress.com/2009/02/21/wmic-wildcard-search-using-like-and/
+ However, we're not even guaranteed that every perl command GS launches will contain the collection name
+ Nor do we want to kill all perl processes that GS launches with bin\windows\perl\bin\perl, though this works:
+    wmic PATH win32_process Where "CommandLine like '%bin%windows%perl%bin%perl%'" Call Terminate
+ The above could kill GS perl processes we don't intend to terminate, as they're not spawned by the particular
+ Process we're trying to terminate from the root down.
+	
+ Solution: We can use taskkill or the longstanding tskill or wmic to kill a process by ID. Since we can
+ kill an external process that SafeProcess launched OK, and only have trouble killing any child processes
+ it launched, we need to know the pids of the child processes. 
+ 
+ We can use Windows' wmic to discover the childpids of a process whose id we know.
+ And we can use JNA to get the process ID of the external process that SafeProcess launched.
+ 
+ To find the processID of the process launched by SafeProcess,
+ need to use Java Native Access (JNA) jars, available jna.jar and jna-platform.jar.
+    http://stackoverflow.com/questions/4750470/how-to-get-pid-of-process-ive-just-started-within-java-program
+    http://stackoverflow.com/questions/35842/how-can-a-java-program-get-its-own-process-id
+    http://www.golesny.de/p/code/javagetpid
+    https://github.com/java-native-access/jna/blob/master/www/GettingStarted.md
+ We're using JNA v 4.1.0, https://mvnrepository.com/artifact/net.java.dev.jna/jna
+  
+ WMIC can show us a list of parent process id and process id of running processes, and then we can
+ kill those child processes with a specific process id.
+    https://superuser.com/questions/851692/track-which-program-launches-a-certain-process
+    http://stackoverflow.com/questions/7486717/finding-parent-process-id-on-windows
+ WMIC can get us the pids of all childprocesses launched by parent process denoted by parent pid.
+ And vice versa:
+    if you know the parent pid and want to know all the pids of the child processes spawned:
+        wmic process where (parentprocessid=596) get processid
+    if you know a child process id and want to know the parent's id:
+        wmic process where (processid=180) get parentprocessid
+ 
+ The above is the current solution.
+ 
+ Eventually, instead of running a windows command to kill the process ourselves, consider changing over to use
+    https://github.com/flapdoodle-oss/de.flapdoodle.embed.process/blob/master/src/main/java/de/flapdoodle/embed/process/runtime/Processes.java 
+ (works with Apache license, http://www.apache.org/licenses/LICENSE-2.0)
+ This is a Java class that uses JNA to terminate processes. It also has the getProcessID() method. 
+ 
+ Linux ps equivalent on Windows is "tasklist", see
+    http://stackoverflow.com/questions/4750470/how-to-get-pid-of-process-ive-just-started-within-java-program
+
+*/
 
 // http://stackoverflow.com/questions/4750470/how-to-get-pid-of-process-ive-just-started-within-java-program
-// http://www.golesny.de/p/code/javagetpid
-// Need to use JNA (Java Native Access), http://stackoverflow.com/questions/35842/how-can-a-java-program-get-its-own-process-id
-// Got 4.1.0 from https://mvnrepository.com/artifact/net.java.dev.jna/jna/4.1.0 and https://mvnrepository.com/artifact/net.java.dev.jna/jna-platform/4.1.0
-// (see also https://mvnrepository.com/artifact/net.java.dev.jna/jna), since github's GettingStarted doc talks about downloading jna.jar but never provides it. 
-// See https://github.com/java-native-access/jna/blob/master/www/GettingStarted.md
+// Uses Java Native Access, JNA
 public static long getProcessID(Process p)
 {
@@ -511,21 +583,4 @@
 }
 
-// UNUSED - only kills toplevel process, same as p.destroy().
-// Call destroyProcess(p) instead to kill any childprocesses launched
-static void killProcessIfOnWindows(Process p) {
-	if(!Utility.isWindows()) return;
-	
-	// only on Windows do we need to still kill perl despite process.destroy()
-	// http://stackoverflow.com/questions/4750470/how-to-get-pid-of-process-ive-just-started-within-java-program
-	// http://www.golesny.de/p/code/javagetpid
-	long processID = getProcessID(p);
-	
-	SafeProcess.killWinProcessWithID(processID);	
-}
-
-
-// Eventually move to using https://github.com/flapdoodle-oss/de.flapdoodle.embed.process/blob/master/src/main/java/de/flapdoodle/embed/process/runtime/Processes.java ?
-// Works with Apache license, http://www.apache.org/licenses/LICENSE-2.0
-
 
 // stackoverflow.com/questions/1835885/send-ctrl-c-to-process-open-by-java	
@@ -535,25 +590,12 @@
 static void killWinProcessWithID(long processID) {
 	
-	// check if we have taskkill or tskill
-	String cmd = null;
-	if (SafeProcess.isAvailable("wmic")) {
-		// https://isc.sans.edu/diary/Windows+Command-Line+Kung+Fu+with+WMIC/1229
-		cmd = "wmic process " + processID + " delete"; // like "kill -9" on Windows
-	} else if(SafeProcess.isAvailable("taskkill")) {
-		cmd = "taskkill /f /t /PID " + processID;
-	} else { //if(SafeProcess.isAvailable("tskill")) { can't check availability since "which tskill" doesn't ever succeed
-		cmd = "tskill " + processID; // https://ss64.com/nt/tskill.html
-	}
-	cmd = "tskill " + processID;
+	String cmd = SafeProcess.getWinProcessKillCmd(processID);
+	if (cmd == null) return;
 	
 	try{		
-		// Despite the /T flag, the following still doesn't kill subprocesses. Taskkill is also not available
-		// universally. Try alternates tskill and pskill, though they too don't kill further processes launched by p.
-		log("Attempting to taskkil process with ID " + processID);
-		//Runtime.getRuntime().exec("taskkill /f /t /PID " + processID);
-		SafeProcess proc = new SafeProcess(cmd);
-			// /f Specifies to forcefully terminate the process(es).
-			//  /t Terminates the specified process and any child processes which were started by it.
-		int exitValue = proc.runProcess(); // no IOstreams for Taskkill, but for "wmic process pid delete" there is output that needs flushing
+		log("\tAttempting to terminate Win subprocess with pid: " + processID);
+		SafeProcess proc = new SafeProcess(cmd);			
+		int exitValue = proc.runProcess(); // no IOstreams for Taskkill, but for "wmic process pid delete"
+			// there is output that needs flushing, so don't use runBasicProcess()
 			
 	} catch(Exception e) {
@@ -562,25 +604,17 @@
 }
 
+
 // On linux and mac, p.destroy() suffices to kill processes launched by p as well.
-// On Windows we need to do more work, since processes launched by p remain around executing until termination otherwise.
-// e.g. full-import.pl may be terminated with p.destroy(), but it launches import.pl which is left running until termination.
+// 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.
 static void destroyProcess(Process p) {
-	//killProcessIfOnWindows(p); // doesn't work: only terminates first level process launched, same as p.destroy()
-	//p.destroy();
-	//killAnyPerl(); // kills any running perl, even those not launched by GS
-	
+	// If it isn't windows, process.destroy() terminates any child processes too
 	if(!Utility.isWindows()) {
 		p.destroy();
 		return;
-	}
-	
-	
-	
-	// We don't want to kill all perl running processes. Just the ones whose command we sort of know and can see in TaskManager
-	// under the "Command Line" column of the Processes tab.
-	// This is possible with wmic, available since xp. First check wmic is really present on this Windows machine
-	// https://superuser.com/questions/52159/kill-a-process-with-a-specific-command-line-from-command-line
+	}	
+
 	if(!SafeProcess.isAvailable("wmic")) {
-		log("wmic used to kill subprocesses is not available. Unable to terminate subprocesses...");
+		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");
 		
@@ -588,53 +622,43 @@
 		p.destroy();
 		return;
-	}
-	
-	
-	// get the process id of the process we launched
-	long processID = SafeProcess.getProcessID(p);
-	
-	
-	// What we want to be able to do on Windows is to kill a process by its command line as seen in Task Manager for that process.
-	// full-import.pl launches import.pl, full-build will launch buildcol.pl and activate.pl.
-	// These tend to have the collection name still at the end, so we can terminate a process knowing the collection name, since
-	// we can use wildcards in our wmic termination command.
-	// However, lucene_passes.pl is launched by buildcol.pl and doesn't have the collection name in its launching cmd line.
-	
-	// Works beatufilly to kill any CommandLine (as seen in TaskManager) that matches
-	//C:>wmic PATH win32_process Where "CommandLine like '%-site%localsite%%lucene-jdbm-demo%'" Call Terminate
-	// https://codeslammer.wordpress.com/2009/02/21/wmic-wildcard-search-using-like-and/
-	// "WMIC Wildcard Search using like and %"
-	// All subsequent perl processes launched by Greenstone are launched with bin\windows\perl\bin\perl. So this works well:
-	// wmic PATH win32_process Where "CommandLine like '%bin%windows%perl%bin%perl%'" Call Terminate
-	// However do we want to terminate all perl processes launched by Greenstone, or only those that we can infer were spawned by
-	// the SafeProcess that we've been running?
-	
-	/*
-	// Works in terminal, but when run through GLI, GLI is terminated too...
-	try{		
-		
-		log("Attempting to use Windows to terminate sub processes");
-		Runtime.getRuntime().exec("wmic PATH win32_process Where \"CommandLine like '%bin%windows%perl%bin%perl%'\" Call Terminate");
-	} catch(Exception e) {
-		log("@@@ Exception attempting to stop perl " + e.getMessage(), e);		
-	}
-	*/
-	
-	// https://superuser.com/questions/851692/track-which-program-launches-a-certain-process
-	// Can get parent process id and process id, and then kill those with child process id
-	// http://stackoverflow.com/questions/7486717/finding-parent-process-id-on-windows
-	// Can get the pids of all childprocesses launched by parent process with parent pid.
-	// And vice versa:
-	// e.g. if you know the parent pid and want to know all the pids of the child processes spawned:
-	// wmic process where (parentprocessid=596) get processid
-	// if you know a child process id and want to know the parent's id:
-	// wmic process where (processid=180) get parentprocessid
-	
-	String[] child_pids = null;
-		
-		
-	log("Attempting to use Windows to terminate sub processes");
-	SafeProcess proc = new SafeProcess("wmic process where (parentprocessid="+processID+") get processid");
+	}	
+	
+	// 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);		
+	log("Attempting to terminate sub processes of Windows process with pid " + processID);
+	terminateSubProcessesRecursively(processID, p);
+	
+}
+
+// Helper function. Only for Windows.
+// Counterintuitively, we're be killing all parent processess and then all child procs and all their descendants
+// as soon as we discover any further process each (sub)process has launched. The parent processes are killed
+// first in each case for 2 reasons: 
+// 1. on Windows, killing the parent process leaves the child running as an orphan anyway, so killing the
+// parent is an independent action, the child process is not dependent on the parent;
+// 2. Killing a parent process prevents it from launching further processes while we're killing off each child process
+private static void terminateSubProcessesRecursively(long parent_pid, Process p) {
+	
+	// Use Windows wmic to find the pids of any sub processes launched by the process denoted by parent_pid	
+	SafeProcess proc = new SafeProcess("wmic process where (parentprocessid="+parent_pid+") get processid");
 	proc.setSplitStdOutputNewLines(true); // since this is windows, splits lines by \r\n
+	int exitValue = proc.runProcess(); // exitValue (%ERRORLEVEL%) is 0 either way.	
+	//log("@@@@ Return value from proc: " + exitValue);
+	
+	// need output from both stdout and stderr: stderr will say there are no pids, stdout will contain pids
+	String stdOutput = proc.getStdOutput();
+	String stdErrOutput = proc.getStdError();
+	
+	
+	// Now we know the pids of the immediate subprocesses, we can get rid of the parent process
+	if(p != null) { // we're the top level process, terminate java way
+		p.destroy();
+	} else { // terminate windows way		
+		SafeProcess.killWinProcessWithID(parent_pid); // get rid off current pid		
+	}
+	
+	// parse the output to get the sub processes' pids	
 	// Output looks like:
 	// ProcessId
@@ -646,18 +670,17 @@
 	// If no children, then STDERR output starts with the following, possibly succeeded by empty lines:
 	// No Instance(s) Available.
-	int exitValue = proc.runProcess(); // exitValue (%ERRORLEVEL%) is 0 either way.	
-	log("@@@@ Return value from proc: " + exitValue);
-	String stdOutput = proc.getStdOutput();
-	// http://stackoverflow.com/questions/691184/scanner-vs-stringtokenizer-vs-string-split
-		
-	log("@@@@ Got output:\n" + stdOutput);
-	if(stdOutput.indexOf("No Instance(s) Available.") == -1) {
-		//int start = stdOutput.indexOf("\n"); // skip first line, which is the column name "ProcessId"
-		//stdOutput = stdOutput.substring(start+1);		
-		//child_pids = stdOutput.split("/(\\r?\\n/)+"); // get rid of newline, so we should only have a list of Long numbers
-		
-		// Now we can get rid of the top level process
-		p.destroy();
+	
+	// base step of the recursion
+	if(stdErrOutput.indexOf("No Instance(s) Available.") != -1) { 
+		//log("@@@@ Got output on stderr: " + stdErrOutput);
+		// No further child processes. And we already terminated the parent process, so we're done
+		return;
+	} else {
+		//log("@@@@ Got output on stdout:\n" + stdOutput);
+	
+		// http://stackoverflow.com/questions/691184/scanner-vs-stringtokenizer-vs-string-split	
+	
 		// find all childprocesses for that pid and terminate them too:
+		Stack<Long> subprocs = new Stack<Long>();
 		Scanner sc = new Scanner(stdOutput);
 		while (sc.hasNext()) {
@@ -666,31 +689,41 @@
 		  } else {
 			long child_pid = sc.nextLong();
-			log("Attempting to terminate child pid: " + child_pid);
-			SafeProcess.killWinProcessWithID(child_pid);
+			subprocs.push(new Long(child_pid));			
 		  }
 		}
-		sc.close();
-	} else {
-		// No child process. Only the top level process needs to be got rid of:
-		p.destroy();
-		
-	}
-	
-	/*
-	
-	// Now we can get rid of the top level process
-	p.destroy();
-	
-	
-	// Then kill each subprocess.
-	
-	if(child_pids != null) {
-		for(int i = 0; i < child_pids.length; i++) {
-			log("Attempting to terminate child pid: |" + child_pids[i] + "|");
-			SafeProcess.killWinProcessWithID(Long.parseLong(child_pids[i].trim()));
+		sc.close();		
+		
+		// recursion step if subprocs is not empty (but if it is empty, then it's another base step)
+		if(!subprocs.empty()) {
+			long child_pid = subprocs.pop().longValue();
+			terminateSubProcessesRecursively(child_pid, null);
+		}	
+	}
+}
+
+// This method should only be called on a Windows OS
+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
+	
+	if(WIN_KILL_CMD == null) {		
+		if(SafeProcess.isAvailable("wmic")) {
+			// https://isc.sans.edu/diary/Windows+Command-Line+Kung+Fu+with+WMIC/1229
+			WIN_KILL_CMD = "wmic process _PROCID_ delete"; // like "kill -9" on Windows
 		}
-	}
-	*/
-	
+		else if(SafeProcess.isAvailable("taskkill")) { // check if we have taskkill or else use the longstanding tskill
+		
+			WIN_KILL_CMD = "taskkill /f /t /PID _PROCID_"; // need to forcefully /f terminate the process				
+				//  /t "Terminates the specified process and any child processes which were started by it."
+				// But despite the /T flag, the above doesn't kill subprocesses.
+		}
+		else { //if(SafeProcess.isAvailable("tskill")) { can't check availability since "which tskill" doesn't ever succeed
+			WIN_KILL_CMD = "tskill _PROCID_"; // https://ss64.com/nt/tskill.html
+		}		
+	}
+	
+	if(WIN_KILL_CMD == null) { // can happen if none of the above cmds were available
+		return null;
+	}
+	return WIN_KILL_CMD.replace( "_PROCID_", Long.toString(processID) );
 }
 
@@ -973,4 +1006,5 @@
     // logger and DebugStream print commands are synchronized, therefore thread safe.
     public static void log(String msg) {
+	if(DEBUG == 0) return;
 	//logger.info(msg);
 
@@ -981,4 +1015,5 @@
 
     public static void log(String msg, Exception e) { // Print stack trace on the exception
+	if(DEBUG == 0) return;
 	//logger.error(msg, e);
 
@@ -991,4 +1026,5 @@
 
     public static void log(Exception e) {
+	if(DEBUG == 0) return;		
 	//logger.error(e);
 
