You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #225 replaced the single-slot frame buffer with a bounded sync_channel(16) and materially reduced the "cursor advances but characters don't render" symptom from #224. However, the symptom still occurs during sustained fast typing. Code review turned up three remaining defects in the server-push pipeline that together can strand a client on stale frames — plus a fourth amplifier specific to the polling client.
1. push_frame drops the NEWEST frame on a full channel (backwards semantics)
src/types.rs:1171-1190
Err(std::sync::mpsc::TrySendError::Full(_)) => {// … creating a fresh channel pair isn't practical here.// Instead, we just skip this frame for this client …true}
When the channel is full, try_send fails and we drop the incoming frame. But the incoming frame carries the latest vt100 state; the 16 frames sitting in the queue are older. Dropping newest means the client is stuck rendering whatever was in the channel 16 slots ago until typing stops and the queue drains.
The doc comment just above the function (lines 1167-1169) explicitly describes the opposite, correct behaviour:
If a client's channel is full, drain the oldest frame first so the newest frame is always delivered
So this is a behaviour/intent mismatch, not just a suboptimal choice. mpsc::sync_channel can't pop from the sender side, so fixing this properly means swapping the transport (e.g. crossbeam_channel::bounded with overwrite semantics, or a Mutex<VecDeque<String>> + Condvar capped at capacity).
2. Writer thread's continue starves the frame drain
src/server/connection.rs:110-142
match resp_rx.recv_timeout(Duration::from_millis(5)){Ok(rrx) => {ifletOk(text) = rrx.recv(){/* write + flush */}whileletOk(rrx) = resp_rx.try_recv(){ifletOk(text) = rrx.recv(){/* write + flush */}}continue;// <-- skips the frame drain below}Err(Disconnected) => break,Err(Timeout) => {}}// 2. Drain all queued frames from the bounded channelwhileletOk(text) = frame_rx.try_recv(){/* write + flush */}
During typing, the client sends dump-state roughly every 10ms (see src/client.rs:2683). Each one enqueues a response on resp_rx. The writer thread processes responses, continues, and never reaches the frame drain until the response queue is empty.
Under sustained typing resp_rx is continuously non-empty, so frames accumulate in the bounded channel faster than they are drained. Once the channel fills, problem #1 kicks in and the client is stuck. Fix is a 1-line change: drop the continue and let the loop fall through to the frame drain every iteration.
3. DumpState double-pushes the same frame to the polling client
src/server/mod.rs:1382-1392
// Push the newly-built frame to ALL persistent clients so// that other attached sessions see the update immediately,// even if they are idle and not polling dump-state.// …crate::types::push_frame(&combined_buf);let _ = resp.send(combined_buf.clone());
For the client that originated the dump-state request, the server does both:
push_frame enqueues the frame on that client's bounded frame channel, and
resp.send queues the same frame on the writer thread's resp_rx.
So every poll causes two copies of a 50-100 KB frame to be written to that client's socket. The duplicate also occupies a slot in the bounded frame channel, which combined with #2 is what drives the queue to capacity.
Fix options: either skip push_frame when handling a DumpState from a persistent client (the response already reaches them), or skip resp.send (since the pushed frame is going out the same socket — but ordering relative to %begin/%end for control-mode clients needs review). Either one alone removes the amplification.
None of these set state_dirty. The server relies on the async PTY reader flipping PTY_DATA_READY (src/pane.rs:1174) before the next iteration picks it up at src/server/mod.rs:628. On the happy path this happens within 1-5 ms, but it's a sub-frame race that can defer the echo by one iteration under load.
Weaker contributor than #1/#2/#3 on its own, but worth closing for predictability.
Suggested ordering
The defects interact, so fixing the cheap ones first is probably enough for most cases:
Summary
PR #225 replaced the single-slot frame buffer with a bounded
sync_channel(16)and materially reduced the "cursor advances but characters don't render" symptom from #224. However, the symptom still occurs during sustained fast typing. Code review turned up three remaining defects in the server-push pipeline that together can strand a client on stale frames — plus a fourth amplifier specific to the polling client.1.
push_framedrops the NEWEST frame on a full channel (backwards semantics)src/types.rs:1171-1190When the channel is full,
try_sendfails and we drop the incoming frame. But the incoming frame carries the latest vt100 state; the 16 frames sitting in the queue are older. Dropping newest means the client is stuck rendering whatever was in the channel 16 slots ago until typing stops and the queue drains.The doc comment just above the function (lines 1167-1169) explicitly describes the opposite, correct behaviour:
So this is a behaviour/intent mismatch, not just a suboptimal choice.
mpsc::sync_channelcan't pop from the sender side, so fixing this properly means swapping the transport (e.g.crossbeam_channel::boundedwith overwrite semantics, or aMutex<VecDeque<String>>+Condvarcapped at capacity).2. Writer thread's
continuestarves the frame drainsrc/server/connection.rs:110-142During typing, the client sends
dump-stateroughly every 10ms (seesrc/client.rs:2683). Each one enqueues a response onresp_rx. The writer thread processes responses,continues, and never reaches the frame drain until the response queue is empty.Under sustained typing
resp_rxis continuously non-empty, so frames accumulate in the bounded channel faster than they are drained. Once the channel fills, problem #1 kicks in and the client is stuck. Fix is a 1-line change: drop thecontinueand let the loop fall through to the frame drain every iteration.3.
DumpStatedouble-pushes the same frame to the polling clientsrc/server/mod.rs:1382-1392For the client that originated the
dump-staterequest, the server does both:push_frameenqueues the frame on that client's bounded frame channel, andresp.sendqueues the same frame on the writer thread'sresp_rx.So every poll causes two copies of a 50-100 KB frame to be written to that client's socket. The duplicate also occupies a slot in the bounded frame channel, which combined with #2 is what drives the queue to capacity.
Fix options: either skip
push_framewhen handling aDumpStatefrom a persistent client (the response already reaches them), or skipresp.send(since the pushed frame is going out the same socket — but ordering relative to%begin/%endfor control-mode clients needs review). Either one alone removes the amplification.4.
SendText/SendKeydon't setstate_dirtysrc/server/mod.rs:1394-1396None of these set
state_dirty. The server relies on the async PTY reader flippingPTY_DATA_READY(src/pane.rs:1174) before the next iteration picks it up atsrc/server/mod.rs:628. On the happy path this happens within 1-5 ms, but it's a sub-frame race that can defer the echo by one iteration under load.Weaker contributor than #1/#2/#3 on its own, but worth closing for predictability.
Suggested ordering
The defects interact, so fixing the cheap ones first is probably enough for most cases:
continuein the writer thread (no colors in bash #2). One line.DumpStatedouble-push to the requesting client (Microsoft Edit Alt Menu and Ctrl commands don't work in psmux #3). Small.push_frame(pwsh is not default installed #1). Defense-in-depth for pathological bursts.state_dirtyonSendText/SendKey/SendPaste(Over SSHlsprints nothing and can't attach #4). Polish.Environment
mainata138a9e(post fix: bounded frame channel to preserve fast typing #225 / fix: fire pane-died/pane-exited hooks with remain-on-exit #228)Related
sync_channel(16)fix