Skip to main content

[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)

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

Back to the top