Skip to content

Close process streams after exit to work around JDK-4311711 hang - #412

Open
elharo wants to merge 3 commits into
masterfrom
fix/process-streams-close
Open

Close process streams after exit to work around JDK-4311711 hang#412
elharo wants to merge 3 commits into
masterfrom
fix/process-streams-close

Conversation

@elharo

@elharo elharo commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

After Process.waitFor() returns, close the process's stdout, stderr, and stdin streams. This works around JDK-4311711 where Process.getInputStream().read() can hang indefinitely even after the process has terminated.

Without this fix, CommandLineUtils.executeCommandLineAsCallable() can hang forever on some JVMs because the StreamPumper threads are blocked in BufferedReader.readLine() on the process's output streams, which never reach EOF.

Fixes #270

@elharo elharo added the bug Something isn't working label Jul 1, 2026
@elharo elharo self-assigned this Jul 1, 2026
@elharo
elharo requested a review from Tibor17 July 1, 2026 14:45
@elharo
elharo requested a review from ascheman July 24, 2026 10:52

@ascheman ascheman left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for tackling this @elharo#270 is a real hang and the instinct (force EOF by closing the stream) is right for the stuck case. But I ran an empirical check against this branch vs master, and the unconditional close introduces a spurious-failure regression on the normal path.

Setup: seq 1 5000 (exits 0, 5000 lines) via executeCommandLine, 60 runs, counting non-zero exits, truncated output, and thrown CommandLineException. Same probe on master and on this branch, JDK 17 and JDK 25.0.3:

branch JDK 17 JDK 25.0.3
master 0/60 exceptions 0/60
this PR 6–7/60 (~10%) 7/60 (~12%)

All failures are CommandLineException: Failure processing stdout on a command that exited 0 with complete output (no truncation observed, so it's not data loss — it's a false failure).

Why: after waitFor() returns, the pumper threads are often still draining buffered output. Closing getInputStream()/getErrorStream() out from under an in-flight readLine() makes it throw IOException, which StreamPumper.run() stores in exception, and call() then rethrows as CommandLineException. A separate probe confirms that when the pumper is idle-blocked on an empty stream (the actual JDK-4311711 hang), the close yields a clean EOF with no exception — so the close only misfires when there's in-flight data.

Suggested direction: make the close a fallback, not the default — waitUntilDone(timeout) first (pumpers reach EOF on their own in the common case, as master shows), and only force-close if they're still stuck past the timeout. Additionally, mark the pumper as intentionally-closed (there's already a disable() hook) so an IOException arising from our close is treated as EOF rather than recorded as a stdout/stderr failure. That keeps the hang fix while removing the false failures.

Happy to contribute the probe as a proper regression test if useful — it reliably reproduces the ~10% false-failure on this branch and stays green on master.

@elharo

elharo commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

If the probe can be contributed as an integration or unit test that would be very useful. Thanks. Does it need to be part of this PR or could it be sent against master?

Resolve conflict in CommandLineUtils: make the JDK-4311711 stream-close a
timeout-based fallback (wait for the pumpers to reach EOF within a grace
period, force EOF by closing the streams only if they are still stuck),
and mark the pumpers as disabled before closing so the resulting
IOException is treated as EOF instead of a stream failure.
@elharo

elharo commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review — the probe reproducing the ~10% spurious 'Failure processing stdout' on a cleanly-exiting process was very helpful. I've merged master into this branch and reworked the close into a fallback as suggested:

  • The process streams are no longer closed unconditionally. After waitFor() the pumpers are first given a grace period (STREAM_EOF_GRACE_PERIOD_MS = 5000) to drain the remaining output and reach EOF on their own (the common case, as on master). Only if they are still stuck after the grace period do we force EOF by closing the streams.
  • Before forcing the close, both pumpers are marked with the existing disable() hook, and StreamPumper.run() now treats an IOException as EOF when disabled, so the close no longer gets recorded as a stdout/stderr failure.
  • Added a timeout-aware AbstractStreamHandler.waitUntilDone(long) for the grace-period wait (plus unit tests for it).
  • Added a regression test (executeCommandLineWithLargeStdoutCompletesWithoutFailure) based on your probe: it runs seq via executeCommandLine repeatedly and asserts exit code 0, complete output, and no exception. I verified it reproduces the spurious CommandLineException against the previous unconditional-close code and stays green with the fallback. It is Unix-only (seq) and skipped elsewhere.

The PR is now up to date with master and green on the matrix. Happy to adjust the grace-period value or anything else.

@elharo
elharo requested a review from ascheman August 4, 2026 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MSHARED-688] [WORKAROUND] executeCommandLineAsCallable hangs reading open std/out on exited Process as JVM bug

2 participants