Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 6 commits into from
+34 −31
Closed

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a264b04
Fix onExit, exitValue and isProcessAlive
djelinski Jun 6, 2024
eddac9e
Fix waitFor
djelinski Jun 6, 2024
6524747
Exclude test for exit code 259 on non-Windows systems
djelinski Jun 6, 2024
45d93b8
exit waitForProcessExit on interrupt
djelinski Jun 7, 2024
e2b457b
More precise comment
djelinski Jun 7, 2024
4ff37ce
waitForProcessExit0 should ignore interrupts
djelinski Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/java.base/windows/native/libjava/ProcessHandleImpl_win.c
View file
Original file line number Diff line number Diff line change
@@ -113,18 +113,10 @@ Java_java_lang_ProcessHandleImpl_waitForProcessExit0(JNIEnv* env,
JNU_ThrowByNameWithLastError(env,
"java/lang/RuntimeException", "GetExitCodeProcess");
} else if (exitValue == STILL_ACTIVE) {
HANDLE events[2];
events[0] = handle;
events[1] = JVM_GetThreadInterruptEvent();
// Interrupt and process termination are treated equally.
// In case of an interrupt, if the process is still active,
// the method returns STILL_ACTIVE (259).
if (WaitForMultipleObjects(sizeof(events)/sizeof(events[0]), events,
FALSE, /* Wait for ANY event */
INFINITE) /* Wait forever */
== WAIT_FAILED) {
if (WaitForSingleObject(handle, INFINITE) /* Wait forever */
== WAIT_FAILED) {
JNU_ThrowByNameWithLastError(env,
"java/lang/RuntimeException", "WaitForMultipleObjects");
"java/lang/RuntimeException", "WaitForSingleObjects");
} else if (!GetExitCodeProcess(handle, &exitValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@djelinski djelinski Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Could you point me to a related bug / documentation / evidence? I tested the WaitForSingleObject version (commit 6524747) on Windows 11, and I didn't observe any problems with shutting down the VM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a test or counter example and don't have a detailed understanding of the blocking needs of Hotspot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reason to retain the checking of GetThreadInterruptEvent is to be belt and suspenders against the Java code changing and opening up a potential error. At the moment, the reaper thread is encapsulated and not likely to get an interrupt, but that might not always be the case and someone changing the Java code might be unaware of the native code details. $.02

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some research, and now I'm pretty confident that WaitForSingleObject is the right option:

  • the thread involved here is a daemon thread, i.e. it doesn't prevent JVM exit
  • the Unix version of waitForProcessExit0 ignores interrupts and signals, it only ends when the wait succeeds or fails
  • I didn't observe any problems with exiting the JVM while waiting on a process handle
  • it's not clear to me what action we would take if the thread was cancelled; completing a CompletableFuture with an InterruptedException doesn't feel right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll buy that argument

JNU_ThrowByNameWithLastError(env,
"java/lang/RuntimeException", "GetExitCodeProcess");