Repository navigation
fix(terminal): process-group teardown, reply budget, resize timeout note - #741
Merged
Merged
Conversation
A resize that changes a dimension moves the frame cursor, so that a frame drawn at the old size is not offered as the repaint answering the new one. A wait_frame that then timed out described the cursor as "the frame this terminal last returned" and gave advice about "this input", though no frame had been returned and nothing typed: the reader was sent to a call that never happened, where the diagnosis is an application that ignored SIGWINCH. The terminal now remembers the size of the resize that last moved the cursor, until wait_frame returns a frame, and the timeout is worded from that: …has not completed a repaint since the resize to 30x6 (1 complete frame in total), and a frame drawn at the old size is not offered. If it does not repaint when resized, assert on the screen with wait_until instead and, when it repainted after the resize and nothing matched, "(1 complete frame since the resize to 30x6, 2 in total)". With no resize since the last returned frame the two messages are unchanged. Before, tests/frames.rs a_wait_frame_timeout_after_a_resize_names_the_resize failed on de7ee37 with "…has not completed a repaint since the frame this terminal last returned (1 complete frame in total). If it does not repaint in response to this input…", the reviewer's frame_msg_resize output. It passes now, with the other 23 tests of the file. The test covers the three states: resized with no repaint, resized with a repaint that did not match, and a frame returned after the resize, where the wording is the frame's again. The message is prose, which STABILITY does not promise, but a test can match it, so the CHANGELOG entry is under Changed with what to match instead. Closes #705 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
`queued_bytes` counts reply bytes waiting for the writer thread, and the
1 MiB bound on undelivered replies (REPLY_QUEUE_BYTES) reads it. Only
`queue_replies` adds to it, and only reply bytes, but the writer took
every byte of each write off it, typed input included, clamped at zero.
So when a test's send had blocked and replies queued behind it, the
application reading the typed bytes cleared the count of replies that
were still waiting, and the bound then admitted as many bytes over
1 MiB as the test had typed.
A write request now carries how many of its bytes are replies
(`reply_bytes`, beside the reply count it already carried), and the
writer takes off exactly those. `queue_replies` adds a request's bytes
before it queues the request, so the count cannot go below zero and the
clamp is gone.
The writer thread's body moves into `writer_loop`, as the reader's is
`reader_loop`, so that a test can drive it with a writer that blocks on
demand. Nothing else about the thread changes: the closure still owns
the duplicate of the master and drops it where it did.
Before, terminal::tests::typed_input_is_not_taken_off_the_reply_budget
failed on the extracted loop with the old arithmetic:
replies still queued must stay on the budget once typed input is written
left: (20000, 0)
right: (20000, 120000)
which is the reviewer's "queued_bytes now 0, reply bytes truly still
queued 120000". It passes now, and goes on to check the bound: with
120,000 reply bytes waiting, exactly 1 MiB - 120,000 more are admitted
and the next reply is dropped and counted. Every step of the test is a
hand-off through a channel; nothing waits on a clock.
The same scenario through a real PTY, on a copy of this branch with one
eprintln in the writer (a raw-mode application that does not read, a
200,000-byte send_str, 20,000 `CSI 6 n`, then the application reads the
200,000 bytes):
wrote 200000 bytes (0 of them reply bytes); queued_bytes now 120000, replies pending 20000
The issue also says the timeout's count of stuck replies under-reported.
It did not: that number is `replies_pending` plus `replies_dropped`,
counted per reply and never from `queued_bytes`, and the failing run
above shows it exact (20000) while the byte count was wrong. What was
broken is the bound alone, so the CHANGELOG entry says only that.
Closes #704
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The comment on the two closes in `Drop` said the writer must close before the master "since the reader polls the master's descriptor while holding this lock". The reader thread never polls the master and never reads under the lifecycle lock: it reads its own duplicate with no lock held, and takes the lock only in `MasterDup::drop`, as it ends. The comment dates from #69 and was not true then either, and whoever next reorders teardown would have relied on it. What the comment now says, each checked against the code: - Both closes are lifecycle edges and happen under the lock (#589). - The order that matters is `unblock_teardown` before the writer: the PTY layer's writer writes an EOT as it is dropped (portable-pty 0.9.0, unix.rs:393-405), and that write must not wait under the lock (#628). `unblock_teardown` also needs the master open. - Between the writer's close and the master's no order is needed. Both, and the reader and writer threads' handles, are duplicates of one open file description (`try_clone`, `try_clone_reader`, `dup_writer`), so the device sees a single close, the last, usually a thread's. - Who takes the lock: `open_pty`, this drop, and `MasterDup::drop`. The code is unchanged. To check the third point rather than argue it, the two closes were swapped and the suites that tear terminals down (teardown, drain, process, descriptors, concurrency, basic, sigchld_ignored) run six times on Linux 7.0: 6 of 6 passed. Not run on macOS, where the claim rests on the duplicates sharing one description. A unit test pins the fact the old comment had wrong: terminal::tests::opening:: the_reader_takes_the_lock_to_close_its_duplicate_and_not_to_read runs `reader_loop` over a probe that notes, at each read and at its drop, whether the thread holds the lock, and requires `read: false` three times and `close: true`. With a guard taken around the read in `reader_loop` it fails with `("read", true)` in each place. Closes #707 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
DESIGN §1 and LIMITATIONS said that on Linux `n_tty` discards input once its 4 KB buffer is full, so an application that never reads its replies gets a timeout with no reply count, and that past "roughly 680 replies" an application has to read as it asks. That is true of canonical mode only. In raw mode, the mode of every application that reads its replies, the write into a full queue blocks on Linux as it does on macOS, the replies stay in the writer, and the next wait's error counts them. Re-measured on Linux 7.0.0-42 with the reviewer's probe, rebuilt against this branch: a python application sets raw mode, or canonical mode with echo off, writes N `CSI 6 n` (six-byte replies), sleeps without reading, then reads whatever arrived and prints the byte count. N raw: read / note canonical: read / note 680 4080 / none 4080 / none 700 4200 / none 4095 / none 1000 6000 / none 4095 / none 2200 13200 / none (twice) 4095 / none 2300 13800 / none (twice) 4095 / none 2350 14100 / 287 replies named 4095 / none 2400 14400 / 353 named (twice) 4095 / none 3000 18000 / 953, 937, 953 named 4095 / none 5000 30000 / 2953 named 4095 / none So in raw mode about 2,300 replies (13.8 KB) fit before the write blocks, the error names the rest, and once the application reads, every reply arrives. In canonical mode the application can read 4,095 bytes, which is 682 replies: the "roughly 680" was canonical mode's figure, and it stays, attributed to it. The rest are discarded with no note. Both documents now say that. drain.rs and backpressure.rs repeated the claim in comments, and drain.rs's `flood` said its 1000 replies came to ~8 KB, past the tty buffer; they are 6 KB, which overflows macOS's queue and fits Linux's raw-mode one. The comments say so; no assertion changes, since the macOS-only assertion there is right, for that reason. Closes #703 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
A member of the program's process group that ignores SIGHUP (a nohup'ed helper, a background job that traps HUP) outlived `Terminal`'s drop and `termlens inspect`, attached to a PTY nobody held. Teardown hung up and killed the child's pid alone, and the kernel's own hang-up, as the session leader exits, goes to the group, which such a member ignores. The child is a session and group leader (`setsid` in spawn), so its pid is its group's id. `stop_child` now goes through `stop_group` on Unix: - the child still running: SIGHUP to the group, then the child's exit is polled for HANGUP_GRACE without reaping (`waitid` + `WNOWAIT`); - the child exited and unreaped, at the drop or within the grace: its zombie holds the group id, so SIGKILL to the group, then the reap; - the child still running after the grace: SIGKILL to the group, reap; - the child reaped elsewhere (`ECHILD`, SIGCHLD ignored): nothing more is signalled, as #630 requires. `Drop` no longer reaps first: that reap is what made the group id unsafe to signal. After `wait_exit` (exit_status set) or a kernel reap (`reaped_elsewhere`) nothing is signalled, as before. `gone_unwaited` becomes `child_state`, which tells Running, Exited and Gone apart from the same `waitid` call (si_pid is zero when WNOHANG finds nothing), so the unsafe-block count stays at eight. `Terminal::signal` still signals the child alone. Windows, and a Unix child without a pid, keep the PTY layer's own `kill`. The stop still runs before the lifecycle lock is taken, so the #628/#633 order is unchanged. Tests, in tests/process.rs, with a helper that traps HUP, says its pid, and stays in the child's group: - drop_stops_a_hangup_ignoring_helper_in_the_child_s_group: the child running at the drop, one that dies of the hang-up and one that ignores it. Before: "helper 201366 outlived the drop of a child that dies on the hang-up". Passes now. - drop_stops_a_hangup_ignoring_helper_after_the_child_exited_unreaped: the test waits with waitid(WNOWAIT) until the child is a zombie, then drops. Before: the helper outlived the drop (old Drop reaped first and signalled nothing). Passes now. - drop_after_wait_exit_signals_nothing: after wait_exit the helper is told to go on and must answer by making a file, which a process with a SIGKILL pending cannot do. Passed before and passes now: the guard against signalling a reaped child's group. The kernel-reaped case stays covered by sigchld_ignored.rs, green. Every helper is killed by a guard if it survives, so a failure leaks nothing. The issue's repro, `termlens inspect sh -c '(trap "" HUP; exec sleep 44) & echo hi; sleep 9'`, now leaves no `sleep 44`. A program that exits with such a helper still running is reaped by inspect's wait first, so its helper still runs on, by design; LIMITATIONS says so. Docs: Drop's rustdoc, DESIGN §1 (teardown), SECURITY.md (what kill(2) and waitid(2) do now; the count is unchanged), LIMITATIONS (nothing is signalled after wait_exit), SKILL rule 7. CHANGELOG under Changed, with the migration: a helper meant to outlive the terminal must leave the program's process group with `setsid`. Fixes #714 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
typed_input_is_not_taken_off_the_reply_budget (#704) runs on every platform, but the test module imported Ordering only on Unix, so the windows-check clippy run failed with E0433 at terminal.rs:5750. The import is now unconditional; clippy passes for x86_64-pc-windows-msvc and for Linux. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five runtime fixes, one commit each.
Changed (each CHANGELOG entry has a migration sentence):
wait_frametimeout after aresizesays "since the frame this terminal last returned" when none was returned #705. Await_frametimeout after aresizenames the resize ("since the resize to 30x6") instead of "the frame this terminal last returned".termlens inspectandTerminal's drop #714 (a decision: signal the group, only while the child is unreaped). On Unix, dropping aTerminal(andtermlens inspectkilling a program at the end of its wait) stops the program's process group, so a member that ignores SIGHUP, such as anohuped helper, no longer outlives it on a PTY nobody holds. The child is a session and group leader, so its pid is the group id, and that id is safe to signal only while the child's zombie holds it. Drop therefore polls the exit withwaitid(WNOWAIT), signals the group, and reaps last. Afterwait_exit, or where the kernel reaped the child (SIGCHLD ignored), nothing is signalled.signalstill reaches the child alone. The unsafe-block count is unchanged. A 30-round loop of the drain, process, sigchld_ignored and teardown suites at 1 and 8 threads passed 240/240.Fixed:
Dropgives a reason the reader thread doesn't have #707. The comment on Drop's close order states the constraint the code has, and a test pins that the reader reads with no lock held.n_tty), and that raw mode blocks and counts them, as on macOS.main is merged in. The merged tree passes fmt, clippy and
cargo test --workspace --all-features.Closes #703
Closes #704
Closes #705
Closes #707
Closes #714