fix: resolve SIGALRM longjmping out of the signal handler - #933
fix: resolve SIGALRM longjmping out of the signal handler #933georglauterbach wants to merge 1 commit into
longjmping out of the signal handler #933Conversation
|
My understanding for the need for reentrancy is so that bash can run a If that is correct, I think a much cleaner approach would be to move settings into a standalone, global, concurrently available object. |
|
Do you mind explain what the SIGALRM stuff is about? I saw that it is mentioned in the claude report you commented, but I didn't read thousands of claude written words. I'd appreciate hearing from you a concise idea of why it's needed for your goal. I thought you were interested in handling SIGUSR2? |
That is correct.
That would also be an approach, although I have not checked the pros and cons here myself. The current design works, but if you think a global object would be cleaner I could also give it a shot. I have just factored out what I had from #931 as promised. IIRC there was no mention yet of such a refactoring.
SIGALRM is the one signal whose Bash handler does not return. Hence, it breaks signal handling while the app is running (because alrm_catcher never returns to the interrupted code, as it longjmps out of the handler, so the app never gets the chance to notice the signal). This is not intrinsically coupled to SIGUSR2, but I'd argue it'd be good to have when flyline handles signals (otherwise SIGALRM will cause problems down the line). |
|
Yes please to moving settings into a global var. I think it will be a lot cleaner. If anything make Flyline reentrant is wrong. We would never want to have two App instances running at once. I didn't review #931 fully. There was a lot of complexity so I didnt do an in depth review. |
|
I think lets split this PR into two. the settings reentrancy part and the sigalrm part. I don't see a reason for them to be together. |
|
I'll drop the reentrancy and only keep the SIGALRM part here for now. Then open another PR for the global object. |
With TMOUT set, Bash arms alarm(TMOUT) around the read and points SIGALRM at alrm_catcher, which longjmps straight out of the signal handler. Delivered while the app owns the terminal, that unwinds past every Rust frame in App::run: no destructor runs, the inline viewport is left half-drawn, and the user is logged out with the terminal still in raw mode. Divert SIGALRM to a handler that only records the delivery for as long as the terminal is in raw mode, exit the main loop when it fires, and re-raise once the terminal is restored and Bash's disposition is back. alrm_catcher then logs out from a clean terminal; a user SIGALRM trap is queued and Bash runs it from parse_command after we return the empty line. The disposition is saved and restored as a full struct sigaction, like SigchldGuard does, so Bash keeps its sa_mask and flags.
2436e56 to
2c3f1ab
Compare
georglauterbach
changed the title
longjmping out of the signal handler
|
#935 is now concerned with refactoring the global settings |
|
Im going to investigate how flyline should handle signals. There is already some signal handling logic around sigchld so I want a consolidated approach. |
|
I found a nice way for flyline to handle traps: b3ab32a |
This change resolves a deadlock that would occur when calling `flyline` from a signal handler (e.g. for `SIGUSR2` or `SIGALRM`) that flyline itself executes (i.e. flyline handles the signal itself). --- - ref #852 - ref #931 - ref #933 --- <details> <summary>Agent Plan</summary> # Suggested Plan ## Global settings object to fix builtin re-entrancy ### Verification of HalFrgrd's claim Confirmed, with one caveat worth stating in the PR. - `Flyline::call` (`src/cli.rs` (`src/cli.rs#L997`)) touches only self.settings — `rg --pcre2 'self\.(?!settings\b)' src/cli.rs` finds nothing. So moving settings out is sufficient to make `flyline_call_command` independent of `FLYLINE_INSTANCE_PTR`. - Caveat: a global behind a lock does not fix anything. App would hold that lock for the whole session and the re-entrant builtin would block on it exactly as it blocks on `FLYLINE_INSTANCE_PTR` today. The global must hand out a borrow per access, never a guard held across a call into Bash's evaluator. - Corroboration that same-thread re-entry is already an accepted reality here: `BASH_LOCK` is a `parking_lot::ReentrantMutex` (`src/shell/bash/symbols.rs` (`src/shell/bash/symbols.rs#L462`)) for this exact reason. - Done per-access, this is strictly sounder than the dropped commit: the app holds a zero-sized handle, not a &mut Settings, so during re-entry there is no live borrow to alias. The thread-local pointer publish had two overlapping `&mut Flyline`. - Flyline never spawns threads (`git grep 'thread::spawn' origin/master -- src crates` is empty; `subshell_ipc::spawn_subshell` forks), so a lock buys nothing but deadlocks. ### Where re-entry happens ```txt ┌───────────────────────────────────────┐ │ │ │ │ │ flyline_get_char │ │ locks FLYLINE_INSTANCE_PTR │ │ │ └───────────────────┬───────────────────┘ ▼ ┌───────────────────────────────────────┐ │ │ │ App::run │ │ │ └───────────────────┬───────────────────┘ ▼ ┌───────────────────────────────────────┐ │ │ │ evaluate_shell_string / decode_prompt │ │ │ └───────────────────┬───────────────────┘ ▼ ┌───────────────────────────────────────┐ │ │ │ Bash evaluator runs user code │ │ │ └───────────────────┬───────────────────┘ ▼ ┌───────────────────────────────────────┐ │ │ │ │ │ flyline builtin ├─────────────────┐ │ flyline_call_command │ after: settings() handle │ │ │ └──────today:─locks─the─same─mutex──────┘ │ ▼ ▼ ┌───────────────────────────────────────┐ ┌───────────────────────────┐ │ │ │ │ │ deadlock │ │ mutates settings, returns │ │ │ │ │ └───────────────────────────────────────┘ └───────────────────────────┘ ``` Three evaluator entry points: `src/app/mod.rs:1213` (`src/app/mod.rs#L1213`) (`run_bash_command`), `src/app/mod.rs:1707` (`src/app/mod.rs#L1707`) (`flycomp` script), and prompt expansion via decode_prompt. `flyline_unget_char` is not reachable during eval because `evalstring` pushes its own input stream. ## Design A lock-free global plus a zero-sized handle that derefs to it. The handle is what keeps the diff small: `App.settings` changes type but all ~136 `self.settings.foo` sites compile unchanged through `Deref`/`DerefMut`, including ones that hand out borrows like `src/app/mod.rs:693` (`src/app/mod.rs#L693`) (-> &mut HistoryManager), which a closure- or guard-based API cannot express. In `src/settings.rs` (`src/settings.rs`), next to Settings: ```rust struct GlobalSettings(std::cell::UnsafeCell<Settings>); // SAFETY: only ever touched from Bash's main thread; flyline spawns no threads // and `spawn_subshell` forks. A lock here would deadlock rather than serialise, // because Bash re-enters the `flyline` builtin on that same thread. unsafe impl Sync for GlobalSettings {} static GLOBAL_SETTINGS: std::sync::LazyLock<GlobalSettings> = ...; /// Handle to the process-wide [`Settings`]. Zero-sized, and materialises a /// borrow only for the duration of each access, so a `flyline set-style` that /// re-enters while `App` holds one of these does not alias a live borrow. /// A borrow derived from a handle MUST NOT be held across a call into Bash's /// evaluator (`evaluate_shell_string`, `decode_prompt`) -- that is where the /// re-entry lands. #[derive(Clone, Copy, Debug, Default)] pub struct SettingsRef; impl Deref for SettingsRef { /* unsafe { &*GLOBAL_SETTINGS.0.get() } */ } impl DerefMut for SettingsRef { /* ... */ } pub fn settings() -> SettingsRef { SettingsRef } ``` `pub(crate) use settings::settings;` in `src/lib.rs` gives `crate::settings()` as requested; the module and the function occupy different namespaces, so crate::settings::Settings keeps working. I audited the three evaluator sites for the "no live borrow" invariant: `run_bash_command` holds none; `poll_flycomp`'s `output_dir()` borrow ends on the line before the eval; the `get_ps1_lines(self.settings.show_animations, ...)` read is a bool copy whose borrow NLL ends before the call. ## Edits - `src/settings.rs`: add the global, SettingsRef, settings(), and the unit test below. - `src/lib.rs`: drop settings from Flyline (keeps content/position); in `Flyline::get`, open with `let mut settings = crate::settings();` and rewrite `self.settings.` to `settings.`; `app::get_command()` loses its argument; `flyline_call_command` becomes `catch_unwind_safe(|| cli::call(words))` and stops locking `FLYLINE_INSTANCE_PTR`; reset the global in `setup_bash_input` where `Flyline::new()` is stored, so enable -d / enable -f still starts from defaults. - `src/cli.rs`: turn `impl Flyline { fn call(&mut self, words) }` into a free `pub(crate) fn call(words) -> c_int` opening with `let mut settings = crate::settings();`, and rewrite the 58 `self.settings.` occurrences to `settings.`. Watch the `settings::AgentModeCommand` paths — multi-segment paths resolve in the type namespace, so the local does not shadow the module, but confirm at compile time. - `src/app/mod.rs`: `get_command()` and `App::new()` lose the parameter (`App::new` opens with `let mut settings = crate::settings();`, body otherwise unchanged); `struct App<'a>` becomes `struct App`; field becomes `settings: SettingsRef`. - Drop the now-unused lifetime in four more impl headers: `src/app/ui.rs:115`, `src/app/auto_close.rs:104` (src/app/auto_close.rs#L104), src/app/actions/keyboard.rs:3296 (src/app/actions/keyboard.rs#L3296), src/completions/tab_completion.rs:1078 (`src/completions/tab_completion.rs#L1078`). Everything else that reads settings stays byte-identical, including &self.settings passed to &Settings parameters (deref coercion) and show_settings(&settings, all). ## Check One unit test in src/settings.rs (src/settings.rs), the smallest thing that fails if the design regresses — it deadlocks if a lock reappears in settings() and asserts if the handle ever starts copying: ```rust /// A re-entrant builtin call must reach the same settings the app is holding. /// The only test that touches the global, so it cannot race the others. #[test] fn reentrant_handles_share_one_settings_instance() { let mut app_view = settings(); // stands in for `App::settings` app_view.frame_rate = 11; settings().frame_rate = 30; // stands in for `flyline --frame-rate 30` assert_eq!(app_view.frame_rate, 30); } ``` Then cargo fmt, cargo clippy --quiet --bins --tests --benches --no-deps -- -D warnings, cargo test --lib. Manual, on bash 5.2.21 with cargo build and enable -f target/debug/libflyline.so flyline: - In-process re-entry: flyline key bind Ctrl+g 'always=runBashCommand("flyline --frame-rate 5")', then Ctrl+g in the app. Hangs the shell today; must return and take effect (flyline settings shows frame_rate: 5). - Forked re-entry, no keystrokes needed: PS1='$(flyline --version)\$ '. The command-substitution child inherits the locked mutex today and hangs; afterwards the prompt renders the version. - No regression: flyline --frame-rate 30 && flyline settings | grep frame_rate, settings still survive across builtin calls and the app honours them. I will not run the Docker matrix (tests/docker_integration_tests.rs); nothing here touches Bash symbols or version gating. </details> --------- Signed-off-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com> Co-authored-by: Hal Frigaard <4559349+HalFrgrd@users.noreply.github.com>
|
Yes exactly. b3ab32a means that flyline will try and run the hooks / traps. But if you run a trap that tries to access the Flyline struct, it will deadlock because Flyline struct is not reentrant. With #935, we the cli call Now it should work! demo: 
Note that I merged 3903eed to get this working. |
|
I think this PR is no longer needed. If the user wants to trap ALRM, they can with e.g. |
|
Awesome! |
2 participants
As discussed in #931, here are two prerequisites that fix issues concerning re-entrancy and TMOUT. The comments are kept concise; I checked what was written and I think it's appropriate given the fact that the logic is not easy to grok.
ref #852
ref #931