On Fri, 7 Jun 2024 14:58:30 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> `GetExitCodeProcess` method is not reliable for checking if a process exited
>> already; it returns 259 (STILL_ACTIVE) if the process hasn't exited yet, but
>> the same value is returned when the process exited with code 259. In order
>> to check if the process exited, we need to check if its handle is in a
>> signaled state using one of the wait methods.
>>
>> This PR fixes the onExit, exitValue, isAlive, and waitFor(timeout) methods
>> to correctly handle the problematic exit code.
>>
>> I haven't fixed the ProcessImpl.toString method. I'm not sure the problem is
>> important enough to justify an extra JNI call in the (probably typical)
>> still-alive case.
>>
>> Tier1-3 testing clean. I modified the existing OnExitTest to cover this case.
>
> src/java.base/windows/native/libjava/ProcessHandleImpl_win.c line 128:
>
>> 126: JNU_ThrowByNameWithLastError(env,
>> 127: "java/lang/RuntimeException", "WaitForMultipleObjects");
>> 128: } else if (!GetExitCodeProcess(handle, &exitValue)) {
>
> This can return STILL_ACTIVE if there is a spurious thread interrupt.
> The interrupt is presumably present to keep the thread from being stuck in a
> blocking windows os call.
>
> The calling code in ProcessHandleImpl is agnostic to platform and would
> report the process had exited.
>
> Can the risk of incorrectly reporting the process exit be mitigated?
>
> If the Thread is legitimately been interrupted, Thread.interrupted() would be
> true and the reaper could exit.
> If false, it could retry the waitForProcessExit().
I only left it here because the wait for interrupt event was already present.
Is being stuck in a blocking os call a bad thing? If not, I can drop the
interrupt event, and wait on the process handle only.
> src/java.base/windows/native/libjava/ProcessImpl_md.c line 471:
>
>> 469: {
>> 470: return WaitForSingleObject((HANDLE) handle, 0) /* don't wait */
>> 471: == WAIT_TIMEOUT;
>
> Would this be better as `isProcessAlive(handle)`?
I don't follow. Could you explain?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631423597
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631424662
