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

[Roger Riggs]

On Fri, 7 Jun 2024 16:05:06 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

>> 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.

I think waiting on JVM_GetThreadInterruptEvent() is necessary during VM shutdown to allow the blocked thread to exit cleanly.

>> 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?

The `WaitForSingleObject(handle, 0)` seems like an indirect way to determine if the process is alive.
Whereas `isProcessAlive(handle)` seems to directly answer the question.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631664185
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631662973