RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259

[Daniel Jeliński]

On Fri, 7 Jun 2024 14:58:30 GMT, Roger Riggs <rriggs at 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