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.ri...@oracle.com
<mailto:roger.ri...@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.ri...@oracle.com <mailto:roger.ri...@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
