[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [jgit-dev] IllegalThreadStateException when calling FS#readPipe if the process lives for a while after closing stderr (race condition)
|
Hello Matthias,
thanks you for looking at the issue. I've pushed the patch to the Gerrit:
https://git.eclipse.org/r/#/c/113023/1
--
Dmitry Pavlenko,
TMate Software,
http://subgit.com/ - git-svn bridge
> On Tue, Dec 5, 2017 at 10:14 PM, Dmitry Pavlenko <pavlenko@xxxxxxxxxxxxx>
>
> wrote:
> > Hello,
> >
> > Recently we've got a report: JGit fails with an exception
> >
> > Exception in thread "Thread-5" java.lang.IllegalThreadStateException:
> > process hasn't exited
> >
> > at java.lang.UNIXProcess.exitValue(UNIXProcess.java:423)
> > at org.eclipse.jgit.util.FS$GobblerThread.setError(FS.java:586)
> > at org.eclipse.jgit.util.FS$GobblerThread.run(FS.java:576)
> >
> > Here's the original issue: https://issues.tmatesoft.com/issue/SGT-1219
> >
> > I'm able to reproduce the problem with the following steps.
> >
> > 1. Create a program that a) prints something to 'stderr'; b) closes
> > 'stderr'; c) doesn't exit for a
> > while after closing.
> >
> > #include <stdio.h>
> >
> > int main() {
> >
> > fprintf(stderr, "line\n");
> > fclose(stderr);
> > while (1) {
> >
> > sleep(1000);
> >
> > }
> > return 0;
> >
> > }
> >
> > I create this file at /tmp/main.c. Compile the code
> >
> > $ cd /tmp
> > $ gcc -o command main.c
> >
> > 2. Create Java class that runs that 'command'. To access protected
> > FS#readPipe method I need to
> >
> > create a subclass:
> > public class SomeClass {
> >
> > private static class FS2 extends FS {
> >
> > public void someMethodToAccessReadPipe() throws
> >
> > CommandFailedException {
> >
> > readPipe(new File("/tmp"), new String[]{"./command"},
> >
> > "UTF-8");
> >
> > }
> >
> > //the lines below are not important as they are not
> >
> > executed.
> >
> > public FS newInstance() {return null;}
> > public boolean supportsExecute() {return false;}
> > public boolean isCaseSensitive() {return false;}
> > public boolean canExecute(File f) {return false;}
> > public boolean setExecute(File f, boolean canExec) {return
> >
> > false;}
> >
> > public boolean retryFailedLockFileCommit() {return false;}
> > protected File discoverGitExe() {return null;}
> > public ProcessBuilder runInShell(String cmd, String[]
> >
> > args) {return null;}
> >
> > }
> >
> > public static void main(String[] args) {
> >
> > try {
> >
> > new FS2().someMethodToAccessReadPipe();
> >
> > } catch (CommandFailedException e) {
> >
> > e.printStackTrace();
> >
> > }
> >
> > }
> >
> > }
> >
> > 3. Run the class, it prints in 100% of cases.
> >
> > Exception in thread "Thread-0" java.lang.IllegalThreadStateException:
> > process hasn't exited
> >
> > at java.lang.UNIXProcess.exitValue(UNIXProcess.java:423)
> > at org.eclipse.jgit.util.FS$GobblerThread.setError(FS.java:586)
> > at org.eclipse.jgit.util.FS$GobblerThread.run(FS.java:576)
> >
> > The explanation. FS#GobblerThread contains the following method:
> > @Override
> > public void run() {
> >
> > StringBuilder err = new StringBuilder();
> > try (InputStream is = p.getErrorStream()) {
> >
> > int ch;
> > while ((ch = is.read()) != -1) {
> >
> > err.append((char) ch);
> >
> > }
> >
> > } catch (IOException e) {
> >
> > if (p.exitValue() != 0) {
> >
> > setError(e, e.getMessage());
> > fail.set(true);
> >
> > } else {
> >
> > // ignore. command terminated faster and stream was
> >
> > just closed
> >
> > }
> >
> > } finally {
> >
> > if (err.length() > 0) {
> >
> > setError(null, err.toString());
> > if (p.exitValue() != 0) {
> >
> > fail.set(true);
> >
> > }
> >
> > }
> >
> > }
> >
> > }
> >
> > The method reads 'stderr' of the process it was created for until 'stderr'
> > is closed or the command
> > exits. If 'stderr' is just closed we get into 'finally' block. "if
> > (err.length() > 0)" condition is
> > true if the command printed to 'stderr' at least something before closing
> > 'stderr'.
> >
> > Then "p.exitValue()" fails because the process is still running.
> >
> > Probably Process#waitFor or Process#isAlive should be called somewhere.
> > Maybe we should put
> > p.waitFor(); immediately after
> >
> > while ((ch = is.read()) != -1) {
> >
> > err.append((char) ch);
> >
> > }
> >
> > block.
>
> I think we should call p.waitFor() as first statement in the finally block
> and also as first statement in
> the catch block catching IOException. In order to avoid leaking a
> GobblerThread if an awkward
> command closes the error stream but doesn't terminate we could consider to
> use waitFor with a timeout.
> If we hit the timeout we should avoid to access the error stream and the
> exitValue but
> set a corresponding error message "command x closed err stream but didn't
> exit within timeout y"
> and set fail to true.
>
> Can you provide a patch ?
>
> -Matthias
