Drain buffered output on Windows after cancellation#296
Conversation
| let (stream, outcome) = self.registerHandle( | ||
| handle, | ||
| processIdentifier: processIdentifier, | ||
| bypassCancellation: true | ||
| ) |
There was a problem hiding this comment.
Instead of "bypassCancellation", which IMO is a bit hacky, can we simply issue one non-blocking read call to read the remaining buffer, since PeekNamedPipe says there are bytes ready to be read? In the unix code path, we simply call read() to drain the remaining bytes. Can we do that here?
There was a problem hiding this comment.
Done. Removed bypassCancellation; registerHandle() surfaces its outcome and read() drains on the cancelled path instead of re-registering through the monitor.
The handle is FILE_FLAG_OVERLAPPED, so the drain is a PeekNamedPipe-gated overlapped ReadFile on a private event, not a bare synchronous read. The handle may already be on the IOCP from an earlier read (and Windows can't unbind it), so the low bit of OVERLAPPED.hEvent is set to keep this completion off the port and away from the monitor (documented opt-out on GetQueuedCompletionStatus).
When a child exits, the run cancels its pending I/O. On Windows, a read issued after that cancellation returned `nil` without consuming bytes already buffered in the pipe, dropping output the child had already written. The kqueue and epoll backends drain the descriptor on this path; the IOCP backend did not. `registerHandle()` now surfaces its registration outcome so `read()` detects the cancelled state before issuing any I/O and drains the buffer itself. `write()` reports a zero-length write on the same path. Both previously issued an overlapped operation and returned without awaiting it, leaving the kernel to write into a buffer the call had already released.
broken-circle
force-pushed
the
drain-buffered-output-96
branch
from
3b408d0 to
d51b8f5
Compare
|
Looks like CI failed due to a flaky Linux test. I'm looking into fixing that test in a separate PR. |
3 participants
Closes #96. The issue was already resolved on non-Windows platforms.
When a child exits, the run cancels its pending I/O via
cancelAsyncIO(). On Windows, a read issued after that cancellation returnednilwithout consuming bytes already buffered in the pipe, so output that the child had already written could be dropped. The kqueue and epoll backends drain the descriptor on this path; the IOCP backend had no equivalent. A read or write reaching the cancelled path also issued an overlapped operation and returned without awaiting it, leaving the kernel to write into a buffer the call had already released.registerHandle()now returns itsRegistrationOutcomesoread()andwrite()can branch on the cancelled state before issuing any I/O:read()drains what's buffered viadrainBufferedDataAfterCancellation(), which polls withPeekNamedPipeand only issues aReadFilewhen data is present, so it can't block waiting for an EOF that may never arrive. That read is registered withbypassCancellationand awaited through the completion port.write()reports a zero-length write, since the subprocess's standard input no longer has a reader.Neither path issues an overlapped operation that it doesn't await.
read()drains on the in-flight cancellation exit as well as when the process was cancelled before the read began. The caller stops on the firstnil, so that exit is the only place left to recover bytes that landed in the buffer with no read pending to receive them. The cost is onePeekNamedPipeat normal EOF, which returnsERROR_BROKEN_PIPEimmediately.