Repository navigation
Call ClosePseudoConsole off the event-loop thread - #36
tonyfettes wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650e95b6f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // empties, and neither side moves again. As with the test below, that | ||
| // failure shows up as the CI job's `timeout-minutes`, not as an assertion. | ||
| #cfg(platform="windows") | ||
| async test "@pty.spawn lets a slow reader reach EOF after the child exits" { |
There was a problem hiding this comment.
On the windows-2022 CI runner, this test deliberately reaches the existing normal-exit path where pty_win32.mbt still calls ClosePseudoConsole on the event-loop thread. Once that call blocks, neither the slow reader nor with_timeout_opt can run, so the test hangs until .github/workflows/ci.yml kills the entire job after ten minutes. Because this commit contains only the reproducer and not the advertised helper-thread implementation, it leaves the Windows CI check unable to pass whenever the regression is successfully reproduced.
Useful? React with 👍 / 👎.
On the normal exit path the wait task calls ClosePseudoConsole on the event-loop thread as soon as the child exits. Before Windows 11 24H2 that call waits for the pseudoconsole to exit, and while it waits no reader task can run, so a pseudoconsole that still has more output than the pipe holds can never finish writing it. The child `type`s two 100x300 screens of text and exits at once while the test reads to EOF. No fix yet: if the analysis is right this commit runs into timeout-minutes on windows-2022. A first attempt with a slow reader and a steady stream of lines did not reproduce it (the run passed on windows-2022 in about a minute); this one replaces it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d64b095 to
85598b1
Compare
The wait task closed the HPCON on the event-loop thread as soon as the child exited. Before Windows 11 24H2 ClosePseudoConsole waits for the pseudoconsole to exit, and the pseudoconsole cannot exit until its remaining output has been read; with the event loop blocked no reader task can run, so a pseudoconsole holding more output than the pipe can take never finishes. Make the call on a short-lived helper thread instead. The event loop keeps serving the reader, and every Windows version gets the "returns immediately" behavior that 24H2 introduced. If the thread cannot be created the call is made in place, as before. The cancellation path still closes the output pipe first: nobody reads it there, so nothing else would unblock the pseudoconsole. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
Follow-up to #35, which fixed the cancellation path and left this one documented as open.
On the normal exit path the wait task calls
ClosePseudoConsoleas soon as the child exits, on the event-loop thread, with the output pipe still open — it has to stay open, because the caller may read up to EOF and EOF only arrives afterClosePseudoConsole.Before Windows 11 24H2 (build 26100) that call waits for the pseudoconsole to exit, and Microsoft's documentation says not to make it on the thread that reads the output pipe. Here the event loop is that thread in effect: while it is blocked no reader task can issue another read, so a pseudoconsole that still holds more output than the pipe can take should never finish writing it.
Change
Two commits, pushed separately so each has its own CI run:
types two 100x300 screens of text and exits at once, while the test reads to EOF and checks the last line and the exit code.ClosePseudoConsoleruns on a short-lived helper thread, so the event loop keeps serving the reader. That gives every Windows version the "returns immediately" behavior 24H2 introduced.Status: not reproduced — the problem is not demonstrated
Nothing win32 here can be run locally (macOS); CI is the only execution.
windows-2022without the fix650e95b, replaced)85598b1)typea screenful, exit at once, normal readerBoth tests were built to leave the pseudoconsole with more unwritten output than the pipe holds at the moment the child exits, and neither hung on
windows-2022without the fix. With the fix, all four jobs pass in both rounds, so the helper thread breaks nothing CI covers — but there is no evidence it fixes anything.Compare #35, where the same method did reproduce the cancellation-path hang (the unfixed run sat in
Testuntiltimeout-minutes).So this PR rests on Microsoft's documentation alone. Recommendation: close it rather than add a thread for an undemonstrated problem. The second test is a reasonable regression test for "a screenful at exit reaches the reader in full" and could be kept on its own.
🤖 Generated with Claude Code