close process streams explicitly after the process exits

[vladak]

With this change the history part of the indexing works again, hurrah!

Also, I renamed the single letter Timer instance and reordered the check so it happens early in the finally block.

[vladak]

I introduced closeStreams() method that is I believe similar to closeQuietly() found in Apache commons or Guava. I left it deliberately private because of google/guava#1118

[ahornace]

Maybe I am missing something but why are you closing the process here and then you are doing the same in finally? I think this is basically redundant...

[vladak]

true, moved the close to the finally block. I never liked setting objects to null anyway.

[ahornace]

Add description to the parameter or delete it. This way it says nothing and just takes one line of code...

[vladak]

added

[ahornace]

process will be destroyed twice. Is it supported? JavaDoc does not say...

[vladak]

this was an oversight, removed.

[ahornace]

Is it wise to do basically the same thing in the exception handler? Won't be the same exception thrown once again?

[vladak]

removed.

[ahornace]

Is this really necessary? Process API was refactored in Java 9 and I have basically the same code in Java 10 as is here. As you can see, there is different handling for Solaris, it might be a good idea to investigate why. Maybe they have good reason.

[vladak]

In Java 8 Netbeans complain if the try/catch block for the IOException is not there.

[ahornace]

No, I mean what is the purpose of closing the streams right before the destroy() call which under the hood does exactly the same. Does destroy() throw an exception which prevents closing the streams?

[tarzanek]

lgtm, after you sort out comments from Adam, feel free to merge ... sanity checking wise it's OK

[idodeclare]

Hi, @vladak. Please see the old OpenGrok IOUtils.close().

[vladak]

@idodeclare thanks, redone. Got rid of the method as a result.

[ahornace]

I looked into the process implementation because I was quite curious how it is done.

I believe it would be safer to call .destroy() instead of directly closing the streams. Mainly because .destroy() method takes into account DeferredCloseInputStream which is used on Solaris.

Rationale for using DeferredCloseInputStream:

    // A FileInputStream that supports the deferment of the actual close
    // operation until the last pending I/O operation on the stream has
    // finished.  This is required on Solaris because we must close the stdin
    // and stdout streams in the destroy method in order to reclaim the
    // underlying file descriptors.  Doing so, however, causes any thread
    // currently blocked in a read on one of those streams to receive an
    // IOException("Bad file number"), which is incompatible with historical
    // behavior.  By deferring the close we allow any pending reads to see -1
    // (EOF) as they did before.

@vladak you know OS stuff much much better than I do so if you think it's okay then I have no problem with it :)

[vladak]

I don't quite like the close() triplet either. That said, while calling destroy() after exitValue() in the finally block would do the trick, it would make an implicit assumption of destroy() closing the streams - the javadoc for it (https://docs.oracle.com/javase/8/docs/api/java/lang/Process.html#destroy--) is fairly silent on what is going on under the hood. Surely it would be nice if Process offered a method to close all of the streams explicitly.

As for the Solaris (and AIX where the constrains are actually worse according to the comments in the UNIXProcess source code) specific semantics, it seems the reason is a difference of what happens if pipe file descriptor is closed while read() syscall on it is in progress. Unlike Linux/BSD, the processReaperExecutor thread does not close the streams so on Solaris one has to close them explicitly or call destroy(). So I think on these 2 systems close() syscall triggers EOF while on Solaris it results in EBADF errno that is possibly translated into IOException. My simple test program with 2 POSIX threads and named pipe (one continuously reading, and the other did close() on the pipe file descriptor after some time) confirms such behaviour on Solaris (EBADF) however I am getting the same on Linux.