Fix IllegalThreadStateException if stderr closed without exiting

If some process executed by FS#readPipe lived for a while after
closing stderr, FS#GobblerThread#run failed with an
IllegalThreadStateException exception when accessing p.exitValue()
for the process which is still alive.

Add Process#waitFor calls to wait for the process completion.

Bug: 528335
Change-Id: I87e0b6f9ad0b995dbce46ddfb877e33eaf3ae5a6
Signed-off-by: Dmitry Pavlenko <pavlenko@tmatesoft.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

3 files changed, 31 insertions, 6 deletions

diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 6e8002ae2e..61ffea4bff 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -120,6 +120,7 @@ classCastNotA=Not a {0}
 cloneNonEmptyDirectory=Destination path "{0}" already exists and is not an empty directory
 closed=closed
 collisionOn=Collision on {0}
+commandClosedStderrButDidntExit=Command {0} closed stderr stream but didn''t exit within timeout {1} seconds
 commandRejectedByHook=Rejected by "{0}" hook.\n{1}
 commandWasCalledInTheWrongState=Command {0} was called in the wrong state
 commitAlreadyExists=exists {0}
@@ -645,6 +646,7 @@ tagAlreadyExists=tag ''{0}'' already exists
 tagNameInvalid=tag name {0} is invalid
 tagOnRepoWithoutHEADCurrentlyNotSupported=Tag on repository without HEAD currently not supported
 theFactoryMustNotBeNull=The factory must not be null
+threadInterruptedWhileRunning="Current thread interrupted while running {0}"
 timeIsUncertain=Time is uncertain
 timerAlreadyTerminated=Timer already terminated
 tooManyCommands=Too many commands
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 0e1a4f3ac4..dd1c7c4ded 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -179,6 +179,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String cloneNonEmptyDirectory;
 	/***/ public String closed;
 	/***/ public String collisionOn;
+	/***/ public String commandClosedStderrButDidntExit;
 	/***/ public String commandRejectedByHook;
 	/***/ public String commandWasCalledInTheWrongState;
 	/***/ public String commitAlreadyExists;
@@ -705,6 +706,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String tagOnRepoWithoutHEADCurrentlyNotSupported;
 	/***/ public String transactionAborted;
 	/***/ public String theFactoryMustNotBeNull;
+	/***/ public String threadInterruptedWhileRunning;
 	/***/ public String timeIsUncertain;
 	/***/ public String timerAlreadyTerminated;
 	/***/ public String tooManyCommands;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
index 3e0b41af34..4fde3f42f7 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java
@@ -564,6 +564,10 @@ public abstract class FS {
 	}
 
 	private static class GobblerThread extends Thread {
+
+		/* The process has 5 seconds to exit after closing stderr */
+		private static final int PROCESS_EXIT_TIMEOUT = 5;
+
 		private final Process p;
 		private final String desc;
 		private final String dir;
@@ -586,15 +590,16 @@ public abstract class FS {
 					err.append((char) ch);
 				}
 			} catch (IOException e) {
-				if (p.exitValue() != 0) {
-					setError(e, e.getMessage());
+				if (waitForProcessCompletion(e) && p.exitValue() != 0) {
+					setError(e, e.getMessage(), p.exitValue());
 					fail.set(true);
 				} else {
 					// ignore. command terminated faster and stream was just closed
+					// or the process didn't terminate within timeout
 				}
 			} finally {
-				if (err.length() > 0) {
-					setError(null, err.toString());
+				if (waitForProcessCompletion(null) && err.length() > 0) {
+					setError(null, err.toString(), p.exitValue());
 					if (p.exitValue() != 0) {
 						fail.set(true);
 					}
@@ -602,11 +607,27 @@ public abstract class FS {
 			}
 		}
 
-		private void setError(IOException e, String message) {
+		@SuppressWarnings("boxing")
+		private boolean waitForProcessCompletion(IOException originalError) {
+			try {
+				if (!p.waitFor(PROCESS_EXIT_TIMEOUT, TimeUnit.SECONDS)) {
+					setError(originalError, MessageFormat.format(
+							JGitText.get().commandClosedStderrButDidntExit,
+							desc, PROCESS_EXIT_TIMEOUT), -1);
+					fail.set(true);
+				}
+			} catch (InterruptedException e) {
+				LOG.error(MessageFormat.format(
+						JGitText.get().threadInterruptedWhileRunning, desc), e);
+			}
+			return false;
+		}
+
+		private void setError(IOException e, String message, int exitCode) {
 			exception.set(e);
 			errorMessage.set(MessageFormat.format(
 					JGitText.get().exceptionCaughtDuringExcecutionOfCommand,
-					desc, dir, Integer.valueOf(p.exitValue()), message));
+					desc, dir, Integer.valueOf(exitCode), message));
 		}
 	}