Skip to content

fix(vt): stop terminal queries blocking Emulator.Write - #953

Open
Rohilalala wants to merge 1 commit into
charmbracelet:mainfrom
Rohilalala:fix/vt-nonblocking-replies
Open

fix(vt): stop terminal queries blocking Emulator.Write#953
Rohilalala wants to merge 1 commit into
charmbracelet:mainfrom
Rohilalala:fix/vt-nonblocking-replies

Conversation

@Rohilalala

Copy link
Copy Markdown

Fixes #939.

The emulator answered queries by writing into an io.Pipe, and a pipe write parks until somebody reads. Ordinary startup traffic asks the terminal about itself, so feeding that to a passive screen model wedged the writer — the issue's repro, verbatim:

emu := vt.NewEmulator(80, 24)
emu.Write([]byte("\x1b[5n")) // never returns

A consumer could avoid it by draining the emulator continuously, but nothing said so, and missing that turned normal output into a deadlock rather than a dropped reply. (I hit this myself writing a test for an unrelated PR — the test deadlocked until I added a reader goroutine.)

Shape

The issue left the shape open and listed three directions; this is the third, "a bounded non-blocking reply path", because it needs no API change and existing drain loops keep working.

Replies go into a bounded buffer that never blocks a write. Two distinctions turned out to matter, both found while testing this:

  • Consumer input is never dropped. Keys, mouse reports and pastes are traffic the consumer chose to send and knows the size of. Only query answers are droppable — that traffic is unbounded and a consumer that never reads is the normal case. Without the split, a paste larger than the bound was discarded outright and the application got a bracketed-paste end with no start.
  • The bound counts only droppable bytes. Otherwise a paste larger than the bound leaves the buffer permanently over it, and every answer arriving afterwards is dropped on arrival — an application waiting on its device-attributes answer would hang, which is the same failure this PR is meant to remove.

Under pressure the oldest replies go first, so a late reader gets the terminal's current state rather than its oldest. Writes are queued whole, so dropping loses complete sequences instead of splicing one in half, and a chunk a reader is part-way through is never dropped out from under it.

The in-band resize notification stays on the never-dropped path: unlike a stale status answer, a size the child never learns is not something it can re-derive.

closed becomes an atomic.Bool, since closing is now the only way to wake a reader parked in Read — it is read on one goroutine while another closes.

Not changed

Emulator.Read still short-circuits to io.EOF once closed, as it did with the pipe. The buffer itself will hand over what it already holds before reporting EOF, so a consumer draining concurrently does not lose the last reply, but the emulator-level behaviour is left alone.

Tests

The reported case; replies surviving until read; the bound holding; whole-sequence eviction; a part-read reply resuming intact; a paste larger than the bound arriving whole; answers still arriving after such a paste; the resize notification surviving pressure; Read blocking until a reply arrives; Close waking a parked reader; and a concurrent reader/writer pair under the race detector.

Every one was checked against a deliberately broken version to confirm it fails for the right reason. That is how the paste-loss, the starvation and the spliced-sequence cases were found in the first place — the first version of this change had all three, and the first version of the "replies stay whole" assertion was vacuous (strings.TrimLeft takes a cutset, not a prefix, so it accepted any scramble).

go test -race -count=3 -shuffle=on passes in vt/, vttest still builds, and golangci-lint run --config ../.golangci.yml ./... reports only the two findings already on main (emulator.go:422, csi.go:38), in files this PR does not touch.

The emulator answered queries by writing into an [io.Pipe], and a pipe
write parks until somebody reads. Ordinary startup traffic asks the
terminal about itself, so feeding that to a passive screen model wedged
the writer:

    emu := vt.NewEmulator(80, 24)
    emu.Write([]byte("\x1b[5n")) // never returns

A consumer could avoid it by draining the emulator continuously, but
nothing said so, and missing that requirement turned normal output into a
deadlock rather than a dropped reply.

Replies now go into a bounded buffer that never blocks a write. What the
consumer sends — keys, mouse reports, pastes — is never dropped, since it
chose to send it and knows how much. Query answers are droppable, because
that traffic is unbounded and a consumer that never reads is the normal
case; under pressure the oldest are dropped so a late reader gets the
terminal's current state rather than its oldest. Writes are queued whole,
so dropping loses complete sequences instead of splicing one, and a chunk
that a reader is part-way through is never dropped out from under it.

Two exceptions to droppable: the in-band resize notification is state the
child cannot re-derive, so it goes on the same path as consumer input;
and the bound counts only droppable bytes, so a paste larger than the
bound cannot leave the buffer permanently over it and starve every answer
that follows.

`closed` becomes an [atomic.Bool]: closing is now the only way to wake a
reader parked in `Read`, so it is read from one goroutine while another
closes.

Tests cover the reported case, replies surviving until read, the bound,
whole-sequence eviction, a part-read reply resuming intact, a paste
larger than the bound arriving whole, answers still arriving after such a
paste, the resize notification surviving pressure, `Read` blocking until
a reply arrives, `Close` waking a parked reader, and a concurrent
reader/writer pair under the race detector.

Fixes charmbracelet#939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vt: terminal queries should not make Emulator.Write block unexpectedly

1 participant