RFR 9: 8086208 : java/lang/ProcessHandle/OnExitTest.java: IllegalThreadStateException: process hasn't exited

[Roger Riggs]

Hi Martin,

On 6/15/2015 6:19 PM, Martin Buchholz wrote:
> OK, it wasn't clear to me this code was only running in a forkjoin pool.
There are several layers in play.
The ProcessHandleImpl.completion() method returns a CF that is completed 
on the ReaperThread.
That CF is unique to the PID.
a. When the process is spawned; (ProcessImpl.initStreams), its the 
CF.handle() contains
the function to store the exitcode and sets hasExited.
b. If onExit is used by the application, handleAsync() creates the CF 
returned from
onExit and is mostly a no-op except for  the exitcode synchronization.

These two completions run concurrently; the purpose of using handleAsync 
for the onExit
case is to ensure the reaper thread only handles lightweight known tasks.
>
> Hmmm ... is there guidance on how tasks in a forkjoin pool should 
> handle InterruptedException?
I haven't unraveled it; it seems to just set a flag in the task and 
otherwise ignore it.
>
> It should be very rare for forkjoin threads to be externally 
> interrupted (task cancellation does not interrupt), so rare that maybe 
> it should be interpreted as an emergency request and failing with 
> InterruptedException is best?
I don't think we have any conventions for that case.
>
> I've obviously lost touch with what this modern Process code is doing 
> ... but:
>
> It looks fishy that you have a future that takes an exitStatus but 
> calls waitFor in its body.
> If the exitStatus is already available, then the process must 
> (should?) have already completed?
I can rename it to unusedExitValue; it is unused.
yes, the process has completed but the ProcessImpl.exitcode field may 
not have been updated (yet).
(There may be no onExit handler if the application didn't call it so it 
can't do any
of the mandatory updates).

>
> It looks fishy to call handle (which gets any exception thrown) but 
> then the exception is discarded.
That's in the CF method signature(s);  there never is any Throwable from 
the process reaper
(ProcessHandleImpl:119).
>
> shouldReap looks weird to me - I'd expect to always have exactly one 
> grim reaper thread per Process that always does the reaping.
Yes but the reaper mechanism is also used for arbitrary processes (that 
are not reaped).
The ProcessHandleImpl.completion() method handles both non-reaping and 
reaping.
>
> But yeah, I already had my chance to review this code ... maybe it's 
> time for me to retire from java.lang.Process.
An extra set of eyes on the code is helpful. You see things I don't.

Thanks, Roger

>
>
> On Mon, Jun 15, 2015 at 2:21 PM, Roger Riggs <Roger.Riggs at oracle.com 
> <mailto:Roger.Riggs at oracle.com>> wrote:
>
>     Hi Martin,
>
>     Since the function is called from the FJ pool; what would result
>     from propagating the interrupt?
>     The FJPool reports the interrupted to the task but otherwise just
>     clears it.
>     It does not affect the completion status.
>
>     Since it is known that the process has terminated and is only
>     waiting for the
>     Thread in ProcessImpl to post the status; it cannot/should not
>     proceed until the
>     exitValue is valid.
>
>     The webrev has been updated to propagate the interrupted status
>     but it will
>     likely be ignored/discarded.
>
>     Roger
>
>
>
>
>     On 6/15/2015 4:46 PM, Martin Buchholz wrote:
>>     if you do get interrupted, the interrupt is swallowed, which
>>     seems wrong.
>>
>>     Other waiting methods have "uninterruptible" variants, that
>>     restore the interrupt status, like
>>     Semaphore.acquireUninterruptibly.  Should there be a
>>     Process.waitForUninterruptibly?
>>
>>     On Mon, Jun 15, 2015 at 1:37 PM, Roger Riggs
>>     <Roger.Riggs at oracle.com <mailto:Roger.Riggs at oracle.com>> wrote:
>>
>>         Please review a fix for a (Unix) race condition for the exit
>>         status of Process.onExit.
>>         And some source cleanup of OnExitTest.
>>
>>         Webrev:
>>         http://cr.openjdk.java.net/~rriggs/webrev-race-8086208/
>>         <http://cr.openjdk.java.net/%7Erriggs/webrev-race-8086208/>
>>
>>         Issue:
>>         https://bugs.openjdk.java.net/browse/JDK-8086208
>>
>>         Roger
>>
>>
>
>