On Wed, 8 Oct 2025 05:26:37 GMT, Alan Bateman <[email protected]> wrote:
>> Roger Riggs has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 22 additional >> commits since the last revision: >> >> - The ProcessCloseTest is modified to be able to test the logic >> around close() waiting for the process to exit and the specified >> behavior of interrupting the waitFor. >> - Close is modified to wait for the process to terminate before returning. >> Please review the revised javadoc. >> As suggested in review comments, waiting for the process to terminate >> allows a more orderly cleanup by the application. >> The streams are closed and close uses `waitFor()` with an unlimited >> timeout >> for the process to terminate. >> While waiting the thread can be `interrupted` to exit the `waitFor`. >> If/when `waitFor` is interrupted, the process is destroyedForcibly on >> all platforms. When close() returns, the thread interrupt will pending >> and can be handled by the caller. >> If a watchdog timeout is desired on close(), a separate thread >> can schedule an interrupt at a suitable time after close is called. >> - Review comment improvements to make expected behavior clearer. >> - Merge branch 'master' into 8364361-process-autocloseable >> - Clarify the use of waitFor after close() or T-W-R exit. >> - Additional review feedback. >> Updated to emphasize reading the data from the streams and calling waitFor >> to allow for normal termination. >> - Merge branch 'master' into 8364361-process-autocloseable >> - Address review comments for code and javadoc in Process, the test and the >> example. >> Reinforced that calling waitFor should occur before calling close to wait >> for termination and get the exitStatus. >> Corrected the error message check for invalid handles on Windows. >> Update test to allow normal completion when exceptions may be expected. >> Correct the expected message on Windows for an invalid handle exception. >> - Update close() to use "terminate" consistently. >> - Remove volatile from "closed" field; updates are guarded by synchronized. >> - ... and 12 more: https://git.openjdk.org/jdk/compare/51c4b310...6f8f7327 > > src/java.base/share/classes/java/lang/Process.java line 235: > >> 233: destroyForcibly(); >> 234: // Re-assert the interrupt >> 235: Thread.currentThread().interrupt(); > > I think this is still problematic as code after the t-w-r can't be guaranteed > that the child has terminated. So I think it needs a loop that ends when > waitFor completes without an exception (destroyForcibly would only be called > on the first interrupt of course). > @AlanBateman can you comment on the possibility of looping indefinitely and > not be interruptible if the OS process cannot be destroyed. AutoCloseable and Closeable.close are specified to release the resources. If Process.close completes normally (no exception) then users should expect that all resources have been released and the process has terminated. In the current proposal, and assuming no interrupt, close waits indefinitely for the process to terminate. If close completes normally then you know the process has terminated. Someone might put code after the t-w-r to delete files or do other cleanup. It's similar to ExecutorService where close waits for all tasks to complete. So I think this part is okay. If interrupted while waiting then close should continue to wait or throw. I think the current proposal, to complete normally but leave the process behind, is problematic and surprising. Having close throw (probably IOException) is a bit problematic too as there isn't much you can do. We regularly see libraries ignoring the IOException thrown by close, including from writable streams where close is screaming that it was unable to flush buffered bytes. So while it wouldn't be terrible to throw, it may be better to keep close simple and be uninterruptible (meaning it would be complete normally when the process terminates and with the interrupt status set). Working through a few use-cases where you might use t-w-r with Process would help to validate the direction ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2435298830
