Index: main/trunk/greenstone2/perllib/downloaders/WgetDownload.pm
===================================================================
--- main/trunk/greenstone2/perllib/downloaders/WgetDownload.pm	(revision 31880)
+++ main/trunk/greenstone2/perllib/downloaders/WgetDownload.pm	(revision 31920)
@@ -112,4 +112,7 @@
 my ($serverSocket, $read_set);
 
+my $TIMEOUT = 1; # seconds
+my $NUM_TRIES = 10;
+
 # The port this script's server socket will be listening on, to handle 
 # incoming signals from GLI to terminate wget. This is also file global, 
@@ -133,5 +136,5 @@
 	# See http://perldoc.perl.org/perlipc.html#Signals 
 	# Warning on using kill at http://perldoc.perl.org/perlfork.html
-	kill("TERM", $childpid); 
+	kill("TERM", $childpid); # prefix - to signal to kill process group
 
 	# If the SIGTERM sent on Linux calls this handler, we want to make
@@ -261,5 +264,5 @@
 # terminate wget). True if we are running gli, or if the particular type
 # of WgetDownload is *not* OAIDownload (in that case, the original way of 
-# terminating the perl script from Java terminated wget as well).
+# terminating the perl script from Java would terminate wget as well).
 sub dealingWithSockets() {
     my ($self) = @_;
@@ -268,5 +271,7 @@
 }
 
-
+# Shouldn't use double quotes around wget path after all? See final comment at
+# http://www.perlmonks.org/?node_id=394709
+# http://coldattic.info/shvedsky/pro/blogs/a-foo-walks-into-a-bar/posts/63
 sub useWget
 {
@@ -308,7 +313,13 @@
 
     my $wget_file_path = &FileUtils::filenameConcatenate($ENV{'GSDLHOME'}, "bin", $ENV{'GSDLOS'}, "wget");
-    $command = "\"$wget_file_path\" $cmdWget";
-    #print STDOUT "Command is: $command\n"; # displayed in GLI output
-    #print STDERR "Command is: $command\n"; # goes into ServerInfoDialog
+    # compose the command as an array rather than as a string, to preserve spaces in the filepath
+    # because single/double quotes using open3 seem to launch a shell, see final comment at 
+    # http://www.perlmonks.org/?node_id=394709 and that ends up causing problems in terminating wget
+    # as 2 processes then got launched which don't have parent-child pid relationship.
+    my @commandargs = split(' ', $cmdWget);
+    unshift(@commandargs, $wget_file_path);
+    $command = "$wget_file_path $cmdWget";
+#    print STDOUT "Command is: $command\n"; # displayed in GLI output
+#    print STDERR "Command is: $command\n"; # goes into ServerInfoDialog
     
     # Wget's output needs to be monitored to find out when it has naturally terminated.
@@ -316,5 +327,5 @@
     # On linux, 2>&1 launches a subshell which then launches wget, meaning that killing
     # the childpid does not kill wget on Linux but the subshell that launched it instead. 
-    # Therefore, we use open3. Though the child process wget sends output only to its stdout, 
+    # Therefore, we use open3. Though the child process wget sends output only to its stdout [is this meant to be "stderr"?], 
     # using open3 says chld_err is undefined and the output of wget only comes in chld_out(!)
     # However that may be, it works with open3. But to avoid the confusion of managing and
@@ -326,6 +337,7 @@
     # Both open2 and open3 don't return on failure, but raise an exception. The handling
     # of the exception is described on p.568 of the Perl Cookbook
-    eval { 
-	$childpid = open3($chld_in, $chld_out, $chld_out, $command);
+    eval {
+	#$childpid = open3($chld_in, $chld_out, $chld_out, $command);
+	$childpid = open3($chld_in, $chld_out, $chld_out, @commandargs);	
     };
     if ($@) {
@@ -336,21 +348,123 @@
     }
 
+    # Switching to use IO::Select, which allows timeouts, instead of doing the potentially blocking
+    #     if defined(my $strLine=<$chld_out>)
+    # Google: perl open3 read timeout
+    # Google: perl open3 select() example
+    # https://stackoverflow.com/questions/10029406/why-does-ipcopen3-get-deadlocked
+    # https://codereview.stackexchange.com/questions/84496/the-right-way-to-use-ipcopen3-in-perl
+    # https://gist.github.com/shalk/6988937
+    # https://stackoverflow.com/questions/18373500/how-to-check-if-command-executed-with-ipcopen3-is-hung
+    # http://perldoc.perl.org/IO/Select.html
+    # http://perldoc.perl.org/IPC/Open3.html - explains the need for select()/IO::Select with open3
+    # http://www.perlmonks.org/?node_id=951554
+    # http://search.cpan.org/~dmuey/IPC-Open3-Utils-0.91/lib/IPC/Open3/Utils.pm
+    # https://stackoverflow.com/questions/3000907/wget-not-behaving-via-ipcopen3-vs-bash?rq=1
+
+    # create the select object and add our streamhandle(s)
+    my $sel = new IO::Select;
+    $sel->add($chld_out);
+
+    my $num_consecutive_timedouts = 0;
+    my $error = 0;
     my $loop = 1;
+    
     while($loop)
     {
-	if (defined(my $strLine=<$chld_out>)) { # we're reading in from child process' stdout
-	    if($blnShow) {
-		print STDERR "$strLine\n";
+	# assume we're going to timeout trying to read from child process
+	$num_consecutive_timedouts++;
+
+	
+	# block until data is available on the registered filehandles or until the timeout specified	
+	if(my @readyhandles = $sel->can_read($TIMEOUT)) {
+
+	    $num_consecutive_timedouts = 0; # re-zero, as we didn't timeout reading from child process after all
+	    # since we're in this if statement
+	    
+	    # now there's a list of registered filehandles we can read from to loop through reading from.
+	    # though we've registered only one, chld_out
+	    foreach my $fh (@readyhandles) {
+		my $strLine;
+		#sleep 3;
+		
+		# read up to 4096 bytes from this filehandle fh.
+		# if there is less than 4096 bytes, we'll only get
+		# those available bytes and won't block.  If there 
+		# is more than 4096 bytes, we'll only read 4096 and
+		# wait for the next iteration through the loop to 
+		# read the rest.
+		my $len = sysread($fh, $strLine, 4096);
+		
+		if($len) { # read something
+		    if($blnShow) {
+			print STDERR "$strLine\n";
+		    }
+		    $strReadIn .= $strLine;
+		}
+		else { # error or EOF: (!defined $len || $len == 0)		    
+		    
+		    if(!defined $len) { # error reading			
+			print STDERR "WgetDownload: Error reading from child stream: $!\n";
+			# SHOULD THIS 'die "errmsg";' instead? - no, sockets may need closing
+			$error = 1;
+		    }
+		    elsif ($len == 0) { # EOF			
+			# Finished reading from this filehand $fh because we read 0 bytes.
+			# wget finished, terminate naturally
+			print STDERR "WgetDownload: wget finished\n";
+			#print STDOUT "\nPerl: open3 command, input streams closed. Wget terminated naturally.\n";
+		    }
+
+		    $loop = 0; # error or EOF, either way will need to clean up and break out of outer loop
+		    
+		    # last; # if we have more than one filehandle registered with IO::Select
+		    
+		    $sel->remove($fh); # if more than one filehandle registered, we should unregister all of them here on error		    
+		    
+		} # end else error or EOF
+		
+	    } # end foreach on readyhandles
+	} # end if on can_read
+	
+	if($num_consecutive_timedouts >= $NUM_TRIES) {
+	    $error = 1;
+	    $loop = 0;                          # to break out of outer while loop
+
+	    $num_consecutive_timedouts = 0;
+
+	    print STDERR "WARNING from WgetDownload: wget timed out $NUM_TRIES times waiting for a response\n";
+	    print STDERR "\tThe URL may be inaccessible or the proxy configuration is wrong or incomplete.\n";
+	    #print STDERR "\tConsider cancelling this download?\n";	
+	}
+
+	if($loop == 0) { # error or EOF, either way, clean up
+	    if($error) {
+		$self->{'forced_quit'} = 1;         # subclasses need to know we're quitting
+		
+		if(kill(0, $childpid)) { 
+		    # If kill(0, $childpid) returns true, then the process is running
+		    # and we need to kill it.
+		    close($chld_in);
+		    close($chld_out);
+		    kill('TERM', $childpid); # kill the process group by prefixing - to signal
+
+		    # https://coderwall.com/p/q-ovnw/killing-all-child-processes-in-a-shell-script
+		    # https://stackoverflow.com/questions/392022/best-way-to-kill-all-child-processes
+		    print STDERR "SENT SIGTERM TO CHILD PID: $childpid\n";
+		    
+		    #die "Perl terminated wget after timing out repeatedly and is about to exit\n";
+		}
 	    }
-	    $strReadIn .= $strLine;
-	}
-	else { # wget finished, terminate naturally
-	    #print STDOUT "\nPerl: open2 command, input stream closed. Wget terminated naturally.\n";
-	    close($chld_in);
-	    close($chld_out);
-	    waitpid $childpid, 0;
-	    $loop = 0;
-	    
+	    else { # wget finished (no errors), terminate naturally
+		#print STDOUT "\nPerl: open2 command, input stream closed. Wget terminated naturally.\n";
+		close($chld_in);
+		close($chld_out);
+		waitpid $childpid, 0;		
+	    }
+
+	    # error or not
 	    $childpid = undef;
+	    # Stop monitoring the read_handle and close the serverSocket 
+	    # (the Java end will close the client socket that Java opened)
 	    if(defined $port) {
 		$read_set->remove($serverSocket);
@@ -359,8 +473,11 @@
 	}
 
+	# If we've already terminated, we can get out of the loop
+	#next if($loop == 0);
+	
 	# if we run this script from the command-line (as opposed to from GLI), 
-	# then we're not working with sockets and can therefore can skip the next bits
+	# then we're not working with sockets and can therefore skip the next bits
 	next unless(defined $port);
-
+	
 	# http://www.perlfect.com/articles/select.shtml
 	# "multiplex between several filehandles within a single thread of control, 
@@ -389,10 +506,10 @@
 		    # for it to start up, checking for whether it is running in order to kill it.
 		    for(my $seconds = 1; $seconds <= 5; $seconds++) {
-			if(kill(0, $childpid)) { 
+			if(kill(0, $childpid)) {
 			    # If kill(0, $childpid) returns true, then the process is running
 			    # and we need to kill it.
 			    close($chld_in);
 			    close($chld_out);
-			    kill("TERM", $childpid);
+			    kill("TERM", $childpid); # prefix - to signal to kill process group
 			    
 			    $childpid = undef;
@@ -460,9 +577,12 @@
 
     my $wget_file_path = &FileUtils::filenameConcatenate($ENV{'GSDLHOME'}, "bin", $ENV{'GSDLOS'}, "wget");
-    my $command = "\"$wget_file_path\" $cmdWget";
+    # compose the command as an array for open3, to preserve spaces in any filepath
+    my @commandargs = split(' ', $cmdWget);
+    unshift(@commandargs, $wget_file_path);
+    my $command = "$wget_file_path $cmdWget";
     #print STDOUT "Command is: $command\n";
 
     eval {     # see p.568 of Perl Cookbook
-	$childpid = open3($chld_in, $chld_out, $chld_out, $command);
+	$childpid = open3($chld_in, $chld_out, $chld_out, @commandargs);
     };
     if ($@) {
@@ -478,37 +598,98 @@
     my $line;
 
+    # create the select object and add our streamhandle(s)
+    my $sel = new IO::Select;
+    $sel->add($chld_out);
+    
+    my $num_consecutive_timedouts = 0;
+    my $error = 0;
     my $loop = 1;
     while($loop)
     {
-	if (defined($line=<$chld_out>)) { # we're reading in from child process' stdout
-	    if((defined $blnShow) && $blnShow)
-	    {
-		print STDERR "$line";
+	# assume we're going to timeout trying to read from child process
+	$num_consecutive_timedouts++;
+
+	# block until data is available on the registered filehandles or until the timeout specified	
+	if(my @readyhandles = $sel->can_read($TIMEOUT)) {
+	    $num_consecutive_timedouts = 0; # re-zero, as we didn't timeout reading from child process after all
+	    # since we're in this if statement
+	    
+	    foreach my $fh (@readyhandles) {
+		my $len = sysread($fh, $line, 4096); # read up to 4k from current ready filehandle
+		if($len) { # read something
+		
+		    
+		    if((defined $blnShow) && $blnShow)
+		    {
+			print STDERR "$line";
+		    }
+		    
+		    if ($line =~ m/^Location:\s*(.*?)\s*\[following\]\s*$/i) {
+			my $follow_url = $1;
+			push(@follow_list,$follow_url);
+		    }
+		    
+		    if ($line =~ m/ERROR\s+\d+/) {
+			$error_text .= $line;
+		    }
+		    
+		    $full_text .= $line;
+		} else { # error or EOF
+		    if(!defined $len) { # error reading
+			#print STDERR "WgetDownload: Error reading from child stream: $!\n";
+			$error = 1;
+		    }
+		    elsif ($len == 0) { # EOF, finished with this filehandle because 0 bytes read
+			#print STDERR "WgetDownload: wget finished\n"; # wget terminated naturally
+		    }
+
+		    $loop = 0; # error or EOF, either way will need to clean up and break out of outer loop
+		    
+		    # last; # if we have more than one filehandle registered with IO::Select
+		    
+		    $sel->remove($fh); # if more than one filehandle registered, we should unregister all of them here on error		    
+		} # end else error or EOF
+		
+	    } # end foreach on readyhandles
+	}  # end if on can_read
+
+	if($num_consecutive_timedouts >= $NUM_TRIES) {
+	    $error = 1;
+	    $loop = 0;                          # to break out of outer while loop
+
+	    $num_consecutive_timedouts = 0;
+
+	    #print STDERR "WARNING from WgetDownload: wget timed out $NUM_TRIES times waiting for a response\n";
+	    #print STDERR "\tThe URL may be inaccessible or the proxy configuration is wrong or incomplete.\n";
+	}
+
+	if($loop == 0) { # error or EOF, either way, clean up
+	    
+	    if($error) {
+		$self->{'forced_quit'} = 1;         # subclasses need to know we're quitting
+		
+		if(kill(0, $childpid)) {
+		    # If kill(0, $childpid) returns true, then the process is running
+		    # and we need to kill it.
+		    close($chld_in);
+		    close($chld_out);
+		    kill("TERM", $childpid); # prefix - to signal to kill process group
+		    
+		    #die "Perl terminated wget after timing out repeatedly and is about to exit\n";
+		}
+	    }
+	    else { # wget finished, terminate naturally
+		close($chld_in);
+		close($chld_out);
+		# Program terminates only when the following line is included 
+		# http://perldoc.perl.org/IPC/Open2.html explains why this is necessary
+		# it prevents the child from turning into a "zombie process".
+		# While the wget process terminates without it, this perl script does not: 
+		# the DOS prompt is not returned without it.
+		waitpid $childpid, 0;
 	    }
 	    
-	    if ($line =~ m/^Location:\s*(.*?)\s*\[following\]\s*$/i) {
-		my $follow_url = $1;
-		push(@follow_list,$follow_url);
-	    }
-	    
-	    if ($line =~ m/ERROR\s+\d+/) {
-		$error_text .= $line;
-	    }
-	    
-	    $full_text .= $line;
-	} 
-	else { # wget finished, terminate naturally
-	    #print STDERR "\nPerl: open2 command, input stream closed. Wget terminated naturally.\n";
-	    close($chld_in);
-	    close($chld_out);
-	    # Program terminates only when the following line is included 
-	    # http://perldoc.perl.org/IPC/Open2.html explains why this is necessary
-	    # it prevents the child from turning into a "zombie process".
-	    # While the wget process terminates without it, this perl script does not: 
-	    # the DOS prompt is not returned without it.
-	    waitpid $childpid, 0;
-	    $loop = 0;
-	    
-	    $childpid = undef;
+	    # error or not:
+	    $childpid = undef;	    
 	    if(defined $port) {
 		$read_set->remove($serverSocket);
@@ -518,5 +699,5 @@
 
 	# if we run this script from the command-line (as opposed to from GLI), 
-	# then we're not working with sockets and can therefore can skip the next bits
+	# then we're not working with sockets and can therefore skip the next bits
 	next unless(defined $port);
 
@@ -547,10 +728,10 @@
 		    # for it to start up, checking for whether it is running in order to kill it.
 		    for(my $seconds = 1; $seconds <= 5; $seconds++) {
-			if(kill(0, $childpid)) { 
+			if(kill(0, $childpid)) {
 			    # If kill(0, $childpid) returns true, then the process is running
 			    # and we need to kill it.
 			    close($chld_in);
 			    close($chld_out);
-			    kill("TERM", $childpid);
+			    kill("TERM", $childpid); # prefix - to signal to kill process group
 			    
 			    $childpid = undef;
