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)

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




Back to the top