feat: move settings into a global var - #935
Conversation
Signed-off-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
|
Thanks! Feedback for this PR: georglauterbach#1 Feel free to merge my PR into yours. Then I will merge this. |
Get rid of SettingsRef, make it explicit that settings is global
Merged :) I had enabled "Allow edits my maintainers" for this PR. I am not quite sure, but this should allow you to make changes and hopefully push to this branch yourself. It's the same to me, really, and I'll do whatever you prefer here. Thanks for the feedback! :) |
Ah cool, Ill try that next time, yw! |
This change resolves a deadlock that would occur when calling
flylinefrom a signal handler (e.g. forSIGUSR2orSIGALRM) that flyline itself executes (i.e. flyline handles the signal itself).longjmping out of the signal handler #933Agent Plan
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.rsfinds nothing. So moving settings out is sufficient to makeflyline_call_commandindependent ofFLYLINE_INSTANCE_PTR.FLYLINE_INSTANCE_PTRtoday. The global must hand out a borrow per access, never a guard held across a call into Bash's evaluator.BASH_LOCKis aparking_lot::ReentrantMutex(src/shell/bash/symbols.rs(src/shell/bash/symbols.rs#L462)) for this exact reason.&mut Flyline.git grep 'thread::spawn' origin/master -- src cratesis empty;subshell_ipc::spawn_subshellforks), so a lock buys nothing but deadlocks.Where re-entry happens
┌───────────────────────────────────────┐ │ │ │ │ │ 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) (flycompscript), and prompt expansion via decode_prompt.flyline_unget_charis not reachable during eval becauseevalstringpushes 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.settingschanges type but all ~136self.settings.foosites compile unchanged throughDeref/DerefMut, including ones that hand out borrows likesrc/app/mod.rs:693(src/app/mod.rs#L693) (-> &mutHistoryManager), which a closure- or guard-based API cannot express.
In
src/settings.rs(src/settings.rs), next to Settings:pub(crate) use settings::settings;insrc/lib.rsgivescrate::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_commandholds none;poll_flycomp'soutput_dir()borrow ends on the line before the eval; theget_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); inFlyline::get, open withlet mut settings = crate::settings();and rewriteself.settings.tosettings.;app::get_command()loses its argument;flyline_call_commandbecomescatch_unwind_safe(|| cli::call(words))and stopslocking
FLYLINE_INSTANCE_PTR; reset the global insetup_bash_inputwhereFlyline::new()is stored, so enable -d / enable -f still starts from defaults.src/cli.rs: turnimpl Flyline { fn call(&mut self, words) }into a freepub(crate) fn call(words) -> c_intopening withlet mut settings = crate::settings();, and rewrite the 58self.settings.occurrences tosettings.. Watch thesettings::AgentModeCommandpaths — multi-segment pathsresolve in the type namespace, so the local does not shadow the module, but confirm at compile time.
src/app/mod.rs:get_command()andApp::new()lose the parameter (App::newopens withlet mut settings = crate::settings();, body otherwise unchanged);struct App<'a>becomesstruct App; field becomessettings: SettingsRef.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:
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:
I will not run the Docker matrix (tests/docker_integration_tests.rs); nothing here touches Bash symbols or version gating.