4

This question is about java.lang.Process and its handling of stdin, stdout and stderr.

We have a class in our project that is an extension to org.apache.commons.io.IOUtils. There we have a quiet new method for closing the std-streams of a Process-Object appropriate? Or is it not appropriate?

/**
 * Method closes all underlying streams from the given Process object.
 * If Exit-Code is not equal to 0 then Process will be destroyed after 
 * closing the streams.
 *
 * It is guaranteed that everything possible is done to release resources
 * even when Throwables are thrown in between.
 *
 * In case of occurances of multiple Throwables then the first occured
 * Throwable will be thrown as Error, RuntimeException or (masked) IOException.
 *
 * The method is null-safe.
 */
public static void close(@Nullable Process process) throws IOException {
    if(process == null) {
      return;
    }

    Throwable t = null;

    try {
      close(process.getOutputStream());
    }
    catch(Throwable e) {
      t = e;
    }

    try{
      close(process.getInputStream());
    }
    catch(Throwable e) {
      t = (t == null) ? e : t;
    }

    try{
      close(process.getErrorStream());
    }
    catch (Throwable e) {
      t = (t == null) ? e : t;
    }

    try{
      try {
        if(process.waitFor() != 0){
          process.destroy();
        }
      }
      catch(InterruptedException e) {
        t = (t == null) ? e : t;
        process.destroy();
      }
    }
    catch (Throwable e) {
      t = (t == null) ? e : t;
    }

    if(t != null) {
      if(t instanceof Error) {
        throw (Error) t;
      }

      if(t instanceof RuntimeException) {
        throw (RuntimeException) t;
      }

      throw t instanceof IOException ? (IOException) t : new IOException(t);
    }
}

public static void closeQuietly(@Nullable Logger log, @Nullable Process process) {
  try {
    close(process);
  }
  catch (Exception e) {
    //log if Logger provided, otherwise discard
    logError(log, "Fehler beim Schließen des Process-Objekts (inkl. underlying streams)!", e);
  }
}

public static void close(@Nullable Closeable closeable) throws IOException {
  if(closeable != null) {
    closeable.close();
  }
}

Methods like these are basically used in finally-blocks.

What I really want to know is if I am safe with this implementation? Considering things like: Does a process object always return the same stdin, stdout and stderr streams during its lifetime? Or may I miss closing streams previously returned by process' getInputStream(), getOutputStream() and getErrorStream() methods?

There is a related question on StackOverflow.com: java: closing subprocess std streams?

Edit

As pointed out by me and others here:

  • InputStreams have to be totally consumed. When not done then the subprocess may not terminate, because there is outstanding data in its output streams.
  • All three std-streams have to be closed. Regardless if used before or not.
  • When the subprocess terminates normally everything should be fine. When not then it have to be terminated forcibly.
  • When an exit code is returned by subprocess then we do not need to destroy() it. It has terminated. (Even when not necessarily terminated normally with Exit Code 0, but it terminated.)
  • We need to monitor waitFor() and interrupt when timeout exceeds to give process a chance to terminate normally but killing it when it hangs.

Unanswered parts:

  • Consider Pros and Cons of consuming the InputStreams in parallel. Or must they be consumed in particular order?
Community
  • 1
  • 1
Fabian Barney
  • 14,219
  • 5
  • 40
  • 60
  • Just a note. You have some serious anti-patterns in your code. 1. Try/catch for almost every single statement. 2. Catching everything "Throwable" each time. 3. The nested try statements could as well be one, with two catch statements. – Bjarke Freund-Hansen Aug 16 '11 at 07:35
  • How to go on working closing the other streams when not putting it into separate try-catch blocks? – Fabian Barney Aug 16 '11 at 08:09
  • Nested try-catch-block have to be since Throwable can occur in nested catch-block. For the catching `Throwable`: What would be more suitable? I want to catch everything it is worth going on working to do my best closing resources. I throw one Throwables at the end when catched before. What can I do better here? – Fabian Barney Aug 16 '11 at 08:36
  • Note that this here is an Utility class for closing resources in finally-blocks elsewhere. It's not its job to handle Exceptions. It throws the first occured Exception. The caller have to deal with it. But it's intention is to do everything possible to release resources given by caller. – Fabian Barney Aug 16 '11 at 08:47

3 Answers3

2

An attempt at simplifying your code:

public static void close(@Nullable Process process) throws IOException
{
    if(process == null) { return; }

    try
    {
        close(process.getOutputStream());
        close(process.getInputStream());
        close(process.getErrorStream());

        if(process.waitFor() != 0)
        {
            process.destroy();
        }
    }
    catch(InterruptedException e)
    {
        process.destroy();
    }
    catch (RuntimeException e)
    {
        throw (e instanceof IOException) ? e : new IOException(e);
    }
}

By catching Throwable I assume you wish to catch all unchecked exceptions. That is either a derivative of RuntimeException or Error. However Error should never be catched, so I have replaced Throwable with RuntimeException.

(It is still not a good idea to catch all RuntimeExceptions.)

Bjarke Freund-Hansen
  • 28,728
  • 25
  • 92
  • 135
  • Your code does not try to close the inputstream when Exception while closing outputstream occurs before. Intention while catching Throwable was to give highest possible guarantee to continue the work with closing resources. The first occurred Throwable is thrown at the end, if an Throwable occured. This is done because it's most likely the Throwable you want to have if something went wrong. Why not catching Throwable in this case? Should I catch `Exception` instead? And why? – Fabian Barney Aug 16 '11 at 08:08
  • `IOException` cannot be a `RuntimeException`. So code of last catch-block can be simplified. But your implementation is not doing everything it can to close the streams. And it may throw other Exceptions than `IOException` when this exception occurs in the catch-interruptedException block. +1 anyway because at least someone who thought about it. – Fabian Barney Aug 16 '11 at 08:23
2

As the question you linked to states, it is better to read and discard the output and error streams. If you are using apache commons io, something like,

new Thread(new Runnable() {public void run() {IOUtils.copy(process.getInputStream(), new NullOutputStream());}}).start();
new Thread(new Runnable() {public void run() {IOUtils.copy(process.getErrorStream(), new NullOutputStream());}}).start();

You want to read and discard stdout and stderr in a separate thread to avoid problems such as the process blocking when it writes enough info to stderr or stdout to fill the buffer.

If you are worried about having two many threads, see this question

I don't think you need to worry about catching IOExceptions when copying stdout, stdin to NullOutputStream, since if there is an IOException reading from the process stdout/stdin, it is probably due to the process being dead itself, and writing to NullOutputStream will never throw an exception.

You don't need to check the return status of waitFor().

Do you want to wait for the process to complete? If so, you can do,

while(true) {
     try
     {
         process.waitFor();
         break;
     } catch(InterruptedException e) {
         //ignore, spurious interrupted exceptions can occur
     }

}

Looking at the link you provided you do need to close the streams when the process is complete, but destroy will do that for you.

So in the end, the method becomes,

public void close(Process process) {

    if(process == null) return;

    new Thread(new Runnable() {public void run() {IOUtils.copy(process.getInputStream(), new NullOutputStream());}}).start();
    new Thread(new Runnable() {public void run() {IOUtils.copy(process.getErrorStream(), new NullOutputStream());}}).start();
    while(true) {
        try
        {
            process.waitFor();
            //this will close stdin, stdout and stderr for the process
            process.destroy();
            break;
        } catch(InterruptedException e) {
            //ignore, spurious interrupted exceptions can occur
        }

   }
}
Community
  • 1
  • 1
sbridges
  • 24,960
  • 4
  • 64
  • 71
  • Many thanks for your answer. Gave it a fast read and will look into it later in more detail. First I've to say: You do not know what happened to Process-Object before. In most cases I've actually written to Outputstream, fetched results over the InputStream etc. You said closing streams is not necessary when I did not use them. Well, ignoring the point we cannot know that here. This [link](http://mark.koli.ch/2011/01/leaky-pipes-remember-to-close-your-streams-when-using-javas-runtimegetruntimeexec.html) says you have to even when you not used them. Is he wrong? Any sources? – Fabian Barney Aug 18 '11 at 10:36
  • Thanks for the link, I didn't know you had to close the streams. I ran some tests and you do need to close those streams. However, the destroy() method will close them for you (if you look at the source of destroy, at least for UnixProcess it does, most likely on windows as well). I have updated my answer. – sbridges Aug 18 '11 at 14:31
  • I am feeling a bit "uncomfortable" about two things here, but maybe you make me feeling more comfortable. :-) 1.) The strong believe destroy() will handle the streams appropriate. 2.) Creating non-referenced "blind" threads for consuming the input streams and then maybe hanging around. Another question: When waitFor() returnes normally, then destroy() is just called for closing the streams, right? And when it is interrupted due to timeout then waitFor() throws an exception and destroy() is not executed, but in this case it is strongly needed in order to forcibly terminate the subprocess?! – Fabian Barney Aug 22 '11 at 12:05
  • To ask more precise: What will happen if subprocess won't terminate normally and have to be terminated forcibly? This is not handled in your code or do I miss something? – Fabian Barney Aug 22 '11 at 13:59
  • If you want to terminate the program forcefully, you need to start another thread, have it sleep for a ma wait time, then have that thread call destroy(). There is nothing in my (or your) code that will forcibly kill a process. Your code does forcibly destroy if interrupted, but the interrupt has nothing to do with the process. In my code, waitfor() is called in a loop to handle the InterruptedException. For 2) The non referenced threads will shutdown when the process exits. For 1) it is an implementation detail, but I think it is dependable. – sbridges Aug 23 '11 at 01:09
  • Hmm, I did not know that. I thought the `waitFor()` will be interrupted if a timeout exceeds?! I'll add some code with a timeout for waitFor() then ... Thanks for pointing this out! – Fabian Barney Aug 23 '11 at 07:50
  • 2
    "spurious interrupted exceptions can occur" ??? No they can't. Why do you think so? – Peter Štibraný Jul 20 '16 at 08:44
  • @PeterŠtibraný exactly, there's no such thing as a spurious interrupted exception, and ignoring one is a really poor idea. – laszlok Mar 08 '17 at 15:25
1

Just to let you know what I have currently in our codebase:

public static void close(@Nullable Process process) throws IOException {
  if (process == null) {
    return;
  }

  Throwable t = null;

  try {
    flushQuietly(process.getOutputStream());
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  try {
    close(process.getOutputStream());
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  try {
    skipAllQuietly(null, TIMEOUT, process.getInputStream());
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  try {
    close(process.getInputStream());
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  try {
    skipAllQuietly(null, TIMEOUT, process.getErrorStream());
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  try {
    close(process.getErrorStream());
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  try {
    try {
      Thread monitor = ThreadMonitor.start(TIMEOUT);
      process.waitFor();
      ThreadMonitor.stop(monitor);
    }
    catch (InterruptedException e) {
      t = mostImportantThrowable(t, e);
      process.destroy();
    }
  }
  catch (Throwable e) {
    t = mostImportantThrowable(t, e);
  }

  if (t != null) {
    if (t instanceof Error) {
      throw (Error) t;
    }

    if (t instanceof RuntimeException) {
      throw (RuntimeException) t;
    }

    throw t instanceof IOException ? (IOException) t : new IOException(t);
  }
}

skipAllQuietly(...) consumes complete InputStreams. It uses internally an implementation similar to org.apache.commons.io.ThreadMonitor to interrupt consumption if a given timeout exceeded.

mostImportantThrowable(...) decides over what Throwable should be returned. Errors over everything. First occured higher prio than later occured. Nothing very important here since these Throwable are most probably discarded anyway later. We want to go on working here and we can only throw one, so we have to decide what we throw at the end, if ever.

close(...) are null-safe implementations to close stuff but throwing Exception when something went wrong.

Fabian Barney
  • 14,219
  • 5
  • 40
  • 60