Improve yamux write backpressure#486
Conversation
48b8fe0 to
16ebd01
Compare
|
@Nashatyrev if of interest, you can have a look |
|
I'm a bit nervous about significant changes in common classes (AbstractMuxHandler and MuxChannel). Do we really want to alter Mplex behavior here? |
|
Revamp with Your teammate's feedback was right on all three points — here's how each is addressed
Robustness fixes found during the rework
New tests Overflow → RST + failed promises + closed stream (both stall causes: window exhausted and parent unwritable), child |
… to yamux - Reinstate maxBufferedConnectionWrites (10 MiB default) and its public API on StreamMuxerProtocol.getYamux. The limit is now enforced against the actual bytes pending in the child channel outbound buffers, covering both send-window stalls and parent TCP backpressure stalls (the latter was never bounded before). On overflow the offending stream is reset, its pending writes are failed and the stream channel is closed, releasing the buffers. - Move all writability/backpressure logic (parent writability gating, write buffer watermarks, user-defined writability flags) from the common classes into YamuxHandler. AbstractMuxHandler and MuxChannel keep only neutral extension points, so Mplex behaves exactly as before. - Remove the redundant retainedPendingWrite ref-counting in MuxChannel: the ChannelOutboundBuffer owns pending messages and releases them on remove/fail/close on every path. - Clamp send window arithmetic so cumulative window updates from a misbehaving remote cannot overflow the window counter. - Fix the deferred-disconnect counter not being decremented when a pending write fails, which could leave the FIN frame unsent.
6d07d59 to
5db52d3
Compare
Summary
This PR improves yamux write backpressure by removing yamux’s private outbound data buffer and routing blocked writes
through Netty stream-channel backpressure instead.
Previously, yamux buffered blocked stream writes internally in
ByteBufQueue, controlled bymaxBufferedConnectionWrites. Downstream users had to tune that value, and in Teku it needed to be set as high as 150MiB to avoid write-buffer overflow. That made yamux unexpectedly memory intensive.
With this change,
maxBufferedConnectionWritesis removed. Yamux no longer exposes a connection-level buffered-writeknob. When a stream cannot send because its yamux send window is exhausted, data remains pending in the stream
MuxChanneloutbound buffer, the stream becomes unwritable, and the write futures remain pending. Writes resume whena
WINDOW_UPDATEis received.Changes
maxBufferedConnectionWritesfrom the yamux API.ByteBufQueuesend buffer.WINDOW_UPDATEand parent channel writability changes.ByteBufownership for pending partial writes and ping echo responses.Notes
This should address the main yamux memory/backpressure issue that forced Teku to configure very large
maxBufferedConnectionWritesvalues. Applications should no longer need to choose a yamux buffered-write limit.This also substantially addresses stream-level
Channel.isWritablesupport for muxed streams, related to #282. Itdoes not fully implement the stricter #316 behavior of completing stream write futures only after bytes are written to
the underlying transport, but write futures now remain pending while yamux cannot consume data due to flow-control.