Skip to content

feat: move settings into a global var - #935

Merged
HalFrgrd merged 4 commits into
HalFrgrd:masterfrom
georglauterbach:fix/global-settings-reentrancy
Aug 16, 2026
Merged

feat: move settings into a global var#935
HalFrgrd merged 4 commits into
HalFrgrd:masterfrom
georglauterbach:fix/global-settings-reentrancy

Conversation

@georglauterbach

@georglauterbach georglauterbach commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

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).



Agent 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.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

┌───────────────────────────────────────┐
│                                       │
│                                       │
│            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:

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:

/// 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.

Signed-off-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
@HalFrgrd

Copy link
Copy Markdown
Owner

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
@georglauterbach

Copy link
Copy Markdown
Contributor Author

Thanks! Feedback for this PR: georglauterbach#1

Feel free to merge my PR into yours. Then I will merge this.

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! :)

@HalFrgrd

Copy link
Copy Markdown
Owner

Thanks! Feedback for this PR: georglauterbach#1
Feel free to merge my PR into yours. Then I will merge this.

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!

@HalFrgrd
HalFrgrd merged commit d22ef68 into HalFrgrd:master Aug 16, 2026
@georglauterbach
georglauterbach deleted the fix/global-settings-reentrancy branch August 16, 2026 20:41
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.

2 participants