Skip to content

Fast typing still drops frames after #225: three remaining causes in the push pipeline #233

@tarikguney

Description

@tarikguney

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_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) => {
        if let Ok(text) = rrx.recv() { /* write + flush */ }
        while let Ok(rrx) = resp_rx.try_recv() {
            if let Ok(text) = rrx.recv() { /* write + flush */ }
        }
        continue;   // <-- skips the frame drain below
    }
    Err(Disconnected) => break,
    Err(Timeout) => {}
}
// 2. Drain all queued frames from the bounded channel
while let Ok(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.

4. SendText / SendKey don't set state_dirty

src/server/mod.rs:1394-1396

CtrlReq::SendText(s) => { app.status_message = None; send_text_to_active(&mut app, &s)?; echo_pending_until = Some(Instant::now()); }
CtrlReq::SendKey(k)  => { app.status_message = None; send_key_to_active(&mut app, &k)?;  echo_pending_until = Some(Instant::now()); }
CtrlReq::SendPaste(s) => { send_paste_to_active(&mut app, &s)?; echo_pending_until = Some(Instant::now()); }

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:

  1. Remove the continue in the writer thread (no colors in bash #2). One line.
  2. Stop the DumpState double-push to the requesting client (Microsoft Edit Alt Menu and Ctrl commands don't work in psmux #3). Small.
  3. Swap the transport or add oldest-drop semantics to push_frame (pwsh is not default installed #1). Defense-in-depth for pathological bursts.
  4. Set state_dirty on SendText/SendKey/SendPaste (Over SSH ls prints nothing and can't attach #4). Polish.

Environment

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions