Repository navigation
End and reap a local shell when its tab closes, and drop the unused os and process plugins - #154
Open
Sadykhzadeh wants to merge 3 commits into
Open
Sadykhzadeh wants to merge 3 commits into
Sadykhzadeh wants to merge 3 commits into
Conversation
…s and process plugins Removing a local terminal dropped its portable-pty child without waiting on it, so every closed tab left a zombie for as long as the app ran, and a shell that ignored the PTY hangup kept running. LocalShell now sends the hangup itself and reaps the shell on a thread of its own. A shell whose exit try_wait already collected is never signalled, because its pid may belong to another process by then. tauri-plugin-os and tauri-plugin-process were registered and os:default was granted, but nothing in the frontend imports either plugin, and restarts go through AppHandle::request_restart. They only added code and let the page read OS details it has no use for.
PR Summary by QodoEnd and reap local shells when their tabs close
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo |
portable-pty's kill() sends SIGHUP, polls for up to 200 ms and then kills outright, and the terminal is dropped under the local terminal map's lock. Closing a tab whose shell took its time therefore held up every other local terminal operation, and cut short whatever the shell does on hangup. The whole shutdown now runs on the reaper thread: the shell gets two seconds to exit after the PTY hangs up, and only then is it signalled. If no thread can be started, the shell is ended inline rather than left as a zombie.
The PTY hangup only reaches the shell once every copy of the master is closed, and the output reader keeps one open while it is blocked reading. A shell busy with a foreground command therefore heard nothing until the grace period ran out and portable-pty's kill took over. It now gets SIGHUP as soon as the tab closes, as with a terminal window, and the grace period starts from there.
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.
Closes #153.
What this changes
Local shells are ended and reaped
LocalTerminal::childis now aLocalShell, a small wrapper around the portable-pty child that does the following on drop:kill(): SIGHUP on Unix, which is what closing a terminal window does, andTerminateProcesson Windows.wait()runs on its own short-lived thread, so the async caller is never blocked, even by a shell that ignores SIGHUP.try_waithas already reported, asreap_exiteddoes. portable-pty signals the raw pid without checking, and once the exit is collected that pid may belong to an unrelated process.Jobs the user detached from the shell (
nohup,disown) are not touched, the same as in any terminal emulator.The unit tests that called
child.kill()by hand after everyremoveno longer need to. A new Linux test spawns/bin/sh, drops the terminal, and waits for/proc/<pid>to disappear. That only happens once the process has been reaped, so a zombie would fail it.Unused plugins removed
tauri-plugin-osandtauri-plugin-processare no longer depended on or registered.os:defaultis no longer granted. This overlaps with #123 on the capability line, and whichever merges second resolves it trivially.Test plan
cargo test -p clavyn-desktop --locked(Linux, local): 108 + 2 passed, includingdropping_a_terminal_ends_and_reaps_its_shellcargo update -w --offline: lockfile drops the two plugins and their exclusive depsnode scripts/verify/check-comments.mjs: okTerminateProcess)Need help on this PR? Tag
@codesmith-botwith what you need.