fix(async): tear down connection on writer write failure - #324
Conversation
| // peer sees EOF, then exit the writer task: the read half will | ||
| // subsequently observe EOF and drive the connection teardown. | ||
| let _ = writer.shutdown().await; | ||
| break; |
There was a problem hiding this comment.
Propagate writer failure to every client stream
When multiple client RPCs are in flight, ClientWriter::disconnect removes only the stream associated with the failed message. After this break, ClientWriter::exit signals shutdown, causing Connection::run to take the wait_shutdown branch and skip ClientReader::disconnect, which is the only path that drains and fails all registered streams.
Closing only the socket's write direction does not make the local read half return EOF. The Client also continues to hold the stream map, so its remaining ResultSenders stay alive. Consequently, a request with timeout_nano == 0, or a streaming receive, can wait indefinitely.
Please treat writer failure as a connection-level error: close the outbound queue, reject subsequent sends, and fail every registered client stream before the connection exits.
| // corrupt the peer's parsing. Shut down the write half so the | ||
| // peer sees EOF, then exit the writer task: the read half will | ||
| // subsequently observe EOF and drive the connection teardown. | ||
| let _ = writer.shutdown().await; |
There was a problem hiding this comment.
Drop the entire connection when the transport cannot half-close
On Windows, Tokio's NamedPipeServer::poll_shutdown only calls poll_flush; it does not disconnect the named pipe. Because tokio::io::split keeps the underlying Socket alive until both halves are dropped, shutting down and dropping the writer half is insufficient while the reader half remains active.
ServerWriter::exit sends no per-connection shutdown notification, and ServerReader::wait_shutdown only observes the server-wide shutdown signal. Therefore, after a recoverable write error, the pipe may remain connected even though there is no writer left to send responses.
Please propagate the writer failure back to Connection::run and perform a connection-wide teardown, including dropping the read half and stopping the connection's handlers. This also handles custom transports whose poll_shutdown does not provide true half-close semantics.
There was a problem hiding this comment.
🟡 Changes recommended
Teardown can block before shutdown and leave unrelated in-flight requests unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Stops asynchronous writers after frame-write failures to prevent protocol desynchronization.
Changes:
- Shuts down the write half after errors.
- Exits the writer loop and avoids subsequent success handling.
File summaries
| File | Description |
|---|---|
src/asynchronous/connection.rs |
Adds write-failure connection teardown. |
Review details
Suppressed comments (1)
src/asynchronous/connection.rs:73
- Breaking here makes
ClientWriter::exitwakeClientReader::wait_shutdown, which exits the reader without callingReaderDelegate::disconnect. The delegate only removes the stream for the failed frame, so any other in-flight request remains inClient::streams; a request withtimeout_nano == 0can then wait forever on a sender that is never dropped. Route writer failures through the full disconnect path (or explicitly drain all pending streams) before stopping the reader.
break;
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| error!("write_message got error: {:?}", e); | ||
| sending_msg.send_result(Err(e.clone())); | ||
| writer_delegate.disconnect(&sending_msg.msg, e).await; | ||
| // write_to uses write_all internally, so a failure may have | ||
| // written only part of a frame. From here the frame | ||
| // boundaries for every stream on this connection can no | ||
| // longer be trusted, and writing further messages would only | ||
| // corrupt the peer's parsing. Shut down the write half so the | ||
| // peer sees EOF, then exit the writer task: the read half will | ||
| // subsequently observe EOF and drive the connection teardown. | ||
| let _ = writer.shutdown().await; | ||
| break; |
|
@Tim-Zhang thanks, you're right that the half-close approach was insufficient. |
There was a problem hiding this comment.
Let Connection::run handle all write-failure cleanup
The oneshot channel is the right idea, but the current implementation is more complex than needed. It adds several shutdown paths and many lines of lifecycle logic, while a focused change of a few dozen lines should be enough.
The current flow can also still block:
writer.shutdown().awaitruns before the error is sent toConnection::run. If shutdown never completes, connection cleanup never starts.ClientReader::disconnectwaits while sending errors to bounded stream channels. One full channel can block cleanup of all other streams.
Please keep the flow simple:
- Report the write error to
Connection::runimmediately. - Exit the writer without waiting for
shutdown(). - Let
Connection::runclose the whole connection and stop its handlers. - Fail all client streams without waiting on a full stream channel.
Please also add tests for a pending shutdown and a full stream channel. This should provide reliable teardown for TCP, Windows named pipes, and custom transports with much less code.
The writer task logged write errors but kept looping to send the next message. Because write_to relies on write_all, a failed write may leave a partially written frame on the wire, desynchronizing frame boundaries for every stream multiplexed on the connection; continuing to write then silently corrupts the peer's parsing. Transient errors are especially problematic since the read half observes nothing and the reader-driven teardown never fires. A concrete example is ENOMEM from the write syscall: under high memory fragmentation the kernel can fail a high-order allocation needed to service the write even though the socket itself is healthy, so relying on the peer or the read side to notice is not enough. Treat a write failure as a connection-level fatal error. On failure the writer task reports the error to Connection::run over a oneshot channel and returns immediately; it does NOT wait on writer.shutdown() first, which can block (some transports never complete poll_shutdown) and would delay or prevent cleanup. run() prioritizes that signal with a biased select and calls reader_delegate.disconnect, which fails every registered client stream (a request with timeout_nano == 0 or a streaming receive would otherwise wait forever) and stops the server's connection handlers, then drops the read half on exit. Cleanup no longer relies on half-closing the write direction to surface EOF to the peer or the local read half, which is not portable: on Windows NamedPipeServer::poll_shutdown only flushes, tokio::io::split keeps the socket alive until both halves are dropped, and custom transports may lack half-close entirely. Dropping the writer task drops the write half; the connection is closed by run(). ClientReader::disconnect now fails pending streams with try_send instead of awaiting a bounded channel, so one stream with a full channel (a slow or stalled receiver) can no longer block teardown of the others. The previous per-message WriterDelegate::disconnect only removed the single failed stream and is now redundant with the connection-wide teardown, so it is removed along with its client and server implementations. Add tests for both hazards: a write failure tears the connection down and fails a pending request even when poll_shutdown never completes and the read half never returns, and disconnect does not block on a full stream channel. Signed-off-by: liukai254 <liukai254@jd.com>
I made the following changes：
|
The writer task logged write errors but kept looping to send the next message. Because write_to relies on write_all, a failed write may leave a partially written frame on the wire, desynchronizing frame boundaries for every stream multiplexed on the connection; continuing to write then silently corrupts the peer's parsing.
Transient errors are especially problematic since the read half observes nothing and the reader-driven teardown never fires. A concrete example is ENOMEM from the write syscall: under high memory fragmentation the kernel can fail a high-order allocation needed to service the write even though the socket itself is perfectly healthy, so relying on the peer or the read side to notice is not enough.
On a write error, shut down the write half so the peer sees EOF and break out of the writer loop. The read half then reads EOF and drives the existing connection teardown path. Also restrict send_result(Ok) to the success path so a failed send no longer reports Ok afterward.