On Thu, 6 Jun 2024 18:40:51 GMT, Daniel Jeliński <djelin...@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().
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)`?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631346916
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631322282
