Fix potential pipe deadlock when wslc's stdin doesn't support overlapped IO#14493
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates wslc’s non-TTY console relay path to reduce the risk of output pipe deadlocks when stdin reads can block, by moving stdin relaying out of the MultiHandleWait loop and into a dedicated thread.
Changes:
- Always relay stdin in a dedicated thread for non-TTY processes (instead of conditionally using
MultiHandleWaitfor non-interactive stdin). - Add/update comments describing the deadlock scenario and why a separate stdin relay path is needed.
| // Required because ReadFile() blocks if stdin doesn't support overlapped IO. | ||
| // This can create pipe deadlocks if we get blocked reading stdin while data is available on stdout / stderr. | ||
| // TODO: Will output CR instead of LF's which can confuse the linux app. | ||
| // Consider a custom relay logic to fix this. | ||
| inputThread = std::thread{[&]() { | ||
| try | ||
| { | ||
| wsl::windows::common::relay::InterruptableRelay(GetStdHandle(STD_INPUT_HANDLE), Stdin.get(), exitEvent.get()); | ||
| } | ||
| CATCH_LOG(); | ||
|
|
||
| Stdin.reset(); | ||
| }}; |
There was a problem hiding this comment.
Spawning a dedicated stdin relay thread helps keep stdout/stderr draining, but the thread is still joined via the scope-exit and InterruptableRelay() can block in ReadFile() (as the comment notes) in ways that are not interrupted by exitEvent. In that case RelayNonTtyProcess() can hang forever while joining inputThread after stdout/stderr complete. Consider making the stdin thread cancellation explicit (e.g., cancel synchronous I/O on the thread before join, or use a relay implementation that can be unblocked by the exit event for non-overlapped stdin).
OneBlue
changed the title
dkbennett
left a comment
dkbennett
left a comment
There was a problem hiding this comment.
I cherry picked and confirmed this fix also fixes the issue I had in E2E interactive tests for non-tty 
Summary of the Pull Request
This change fixes a potential pipe deadlock that can happen where stdin is a pipe that doesn't support overlapped IO.
This can be reproduced by:
cat | wslc run -i debian:latest catPR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed