From 7ccf38adc60c12606164655386630b4e296baf13 Mon Sep 17 00:00:00 2001 From: Dmitry Pavlenko Date: Thu, 7 Dec 2017 19:36:35 +0100 Subject: [PATCH] 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 Signed-off-by: Matthias Sohn --- .../eclipse/jgit/internal/JGitText.properties | 2 ++ .../org/eclipse/jgit/internal/JGitText.java | 2 ++ .../src/org/eclipse/jgit/util/FS.java | 33 +++++++++++++++---- 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)); } } -- 2.39.5