From 0f1848055966ba8d5397a8f15549e7222dc44e08 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Thu, 16 May 2024 17:40:23 -0500 Subject: [PATCH] Simplify Parser and EnvStack singletons and clarify thread semantics `Parser` is a single-threaded `!Send`, `!Sync` type and does not need to use `Arc` for anything. We were using it because that's all we had for the parser's `EnvStack`, but though that is *technically* protected internally by a mutex (shared with global EnvStack), there's nothing to say that other parsers with a narrower scope/lifetime on other threads will be necessarily using the same backing mutex. We can safely marshal the existing `Arc` we get from `environment::principal()` into an `Rc` since the underlying reference is always valid. To prove this point, we could have PRINCIPAL_STACK be a static `EnvStack` and have `environment::principal()` use `Arc::from_raw()` to turn that into an `Arc`, but there's no need to factorize this process. --- src/bin/fish_indent.rs | 6 +----- src/env/environment.rs | 36 ++++++++++++------------------------ src/function.rs | 2 +- src/operation_context.rs | 11 +++++------ src/parser.rs | 22 +++++++++++++++------- src/tests/input.rs | 4 ++-- 6 files changed, 36 insertions(+), 45 deletions(-) diff --git a/src/bin/fish_indent.rs b/src/bin/fish_indent.rs index fbced09b5..6d19b7687 100644 --- a/src/bin/fish_indent.rs +++ b/src/bin/fish_indent.rs @@ -990,11 +990,7 @@ enum OutputType { } } OutputType::Ansi => { - colored_output = colorize( - &output_wtext, - &colors, - EnvStack::globals().as_ref().get_ref(), - ); + colored_output = colorize(&output_wtext, &colors, &**EnvStack::globals()); } OutputType::Html => { colored_output = html_colorize(&output_wtext, &colors); diff --git a/src/env/environment.rs b/src/env/environment.rs index 1192b50d5..86704bbf4 100644 --- a/src/env/environment.rs +++ b/src/env/environment.rs @@ -27,7 +27,6 @@ use crate::wutil::{fish_wcstol, wgetcwd, wgettext}; use std::sync::atomic::Ordering; -use lazy_static::lazy_static; use libc::{c_int, uid_t, STDOUT_FILENO, _IONBF}; use once_cell::sync::OnceCell; use std::collections::HashMap; @@ -35,7 +34,6 @@ use std::io::Write; use std::mem::MaybeUninit; use std::os::unix::prelude::*; -use std::pin::Pin; use std::sync::Arc; /// Set when a universal variable has been modified but not yet been written to disk via sync(). @@ -190,7 +188,7 @@ fn lock(&self) -> EnvMutexGuard { /// Return whether we are the principal stack. pub fn is_principal(&self) -> bool { - std::ptr::eq(self, Self::principal().as_ref().get_ref()) + std::ptr::eq(self, &**Self::principal()) } /// Helpers to get and set the proc statuses. @@ -362,13 +360,17 @@ pub fn universal_sync(&self, always: bool) -> Vec { /// A variable stack that only represents globals. /// Do not push or pop from this. - pub fn globals() -> &'static EnvStackRef { - &GLOBALS + pub fn globals() -> &'static Arc { + use std::sync::OnceLock; + static GLOBALS: OnceLock> = OnceLock::new(); + &GLOBALS.get_or_init(|| Arc::new(EnvStack::new())) } /// Access the principal variable stack, associated with the principal parser. - pub fn principal() -> &'static EnvStackRef { - &PRINCIPAL_STACK + pub fn principal() -> &'static Arc { + use std::sync::OnceLock; + static PRINCIPAL_STACK: OnceLock> = OnceLock::new(); + &PRINCIPAL_STACK.get_or_init(|| Arc::new(EnvStack::new())) } pub fn set_argv(&self, argv: Vec) { @@ -408,20 +410,6 @@ fn get_pwd_slash(&self) -> WString { } } -// TODO Remove Pin? -pub type EnvStackRef = Pin>; - -// A variable stack that only represents globals. -// Do not push or pop from this. -lazy_static! { - static ref GLOBALS: EnvStackRef = Arc::pin(EnvStack::new()); -} - -// Our singleton "principal" stack. -lazy_static! { - static ref PRINCIPAL_STACK: EnvStackRef = Arc::pin(EnvStack::new()); -} - /// Some configuration path environment variables. const FISH_DATADIR_VAR: &wstr = L!("__fish_data_dir"); const FISH_SYSCONFDIR_VAR: &wstr = L!("__fish_sysconf_dir"); @@ -539,7 +527,7 @@ fn setup_user(vars: &EnvStack) { fn setup_path() { use crate::libc::{confstr, _CS_PATH}; - let vars = &GLOBALS; + let vars = EnvStack::globals(); let path = vars.get_unless_empty(L!("PATH")); if path.is_none() { // _CS_PATH: colon-separated paths to find POSIX utilities @@ -566,7 +554,7 @@ fn setup_path() { pub static INHERITED_VARS: OnceCell> = OnceCell::new(); pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool) { - let vars = &PRINCIPAL_STACK; + let vars = &**EnvStack::principal(); let env_iter: Vec<_> = std::env::vars_os() .map(|(k, v)| (str2wcstring(k.as_bytes()), str2wcstring(v.as_bytes()))) @@ -725,7 +713,7 @@ pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool // Initialize termsize variables. // PORTING: 3x deref is weird - let termsize = termsize::SHARED_CONTAINER.initialize(&***vars as &dyn Environment); + let termsize = termsize::SHARED_CONTAINER.initialize(vars as &dyn Environment); if vars.get_unless_empty(L!("COLUMNS")).is_none() { vars.set_one(L!("COLUMNS"), EnvMode::GLOBAL, termsize.width.to_wstring()); } diff --git a/src/function.rs b/src/function.rs index 6fa6abbf2..008ebe730 100644 --- a/src/function.rs +++ b/src/function.rs @@ -122,7 +122,7 @@ pub fn load(name: &wstr, parser: &Parser) -> bool { if funcset.allow_autoload(name) { if let Some(path) = funcset .autoloader - .resolve_command(name, EnvStack::globals().as_ref().get_ref()) + .resolve_command(name, &**EnvStack::globals()) { path_to_autoload = Some(path); } diff --git a/src/operation_context.rs b/src/operation_context.rs index 324c34286..2efd4280b 100644 --- a/src/operation_context.rs +++ b/src/operation_context.rs @@ -1,10 +1,8 @@ use crate::common::CancelChecker; use crate::env::EnvDyn; -use crate::env::{EnvStack, EnvStackRef, Environment}; +use crate::env::{EnvStack, Environment}; use crate::parser::{Parser, ParserRef}; use crate::proc::JobGroupRef; -use once_cell::sync::Lazy; -use std::sync::Arc; use crate::reader::read_generation_count; @@ -47,8 +45,6 @@ pub struct OperationContext<'a> { pub cancel_checker: CancelChecker, } -static nullenv: Lazy = Lazy::new(|| Arc::pin(EnvStack::new())); - impl<'a> OperationContext<'a> { pub fn vars(&self) -> &dyn Environment { match &self.vars { @@ -60,7 +56,10 @@ pub fn vars(&self) -> &dyn Environment { // Return an "empty" context which contains no variables, no parser, and never cancels. pub fn empty() -> OperationContext<'static> { - OperationContext::background(&**nullenv, EXPANSION_LIMIT_DEFAULT) + use std::sync::OnceLock; + static NULL_ENV: OnceLock = OnceLock::new(); + let null_env = NULL_ENV.get_or_init(|| EnvStack::new()); + OperationContext::background(null_env, EXPANSION_LIMIT_DEFAULT) } // Return an operation context that contains only global variables, no parser, and never diff --git a/src/parser.rs b/src/parser.rs index e2c5207a0..4c343e85d 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -7,7 +7,7 @@ FilenameRef, ScopeGuarding, PROFILING_ACTIVE, }; use crate::complete::CompletionList; -use crate::env::{EnvMode, EnvStack, EnvStackRef, EnvStackSetResult, Environment, Statuses}; +use crate::env::{EnvMode, EnvStack, EnvStackSetResult, Environment, Statuses}; use crate::event::{self, Event}; use crate::expand::{ expand_string, replace_home_directory_with_tilde, ExpandFlags, ExpandResultCode, @@ -37,7 +37,6 @@ use std::ffi::{CStr, OsStr}; use std::os::fd::{AsRawFd, OwnedFd, RawFd}; use std::os::unix::prelude::OsStrExt; -use std::pin::Pin; use std::rc::{Rc, Weak}; use std::sync::{ atomic::{AtomicIsize, AtomicU64, Ordering}, @@ -312,7 +311,7 @@ pub struct Parser { pub eval_level: AtomicIsize, /// Set of variables for the parser. - pub variables: EnvStackRef, + pub variables: Rc, /// Miscellaneous library data. library_data: RefCell, @@ -333,7 +332,7 @@ pub struct Parser { impl Parser { /// Create a parser - pub fn new(variables: EnvStackRef, is_principal: bool) -> ParserRef { + pub fn new(variables: Rc, is_principal: bool) -> ParserRef { let result = Rc::new_cyclic(|this: &Weak| Self { this: Weak::clone(this), execution_context: RefCell::default(), @@ -405,7 +404,16 @@ pub fn principal_parser() -> &'static Parser { static PRINCIPAL: MainThread> = MainThread::new(OnceCell::new()); &PRINCIPAL .get() - .get_or_init(|| Parser::new(EnvStack::principal().clone(), true)) + // The parser is !Send/!Sync and strictly single-threaded, but we can have + // multi-threaded access to its variables stack (why, though?) so EnvStack::principal() + // returns an Arc instead of an Rc. Since the Arc is + // statically allocated and always valid (not even destroyed on exit), we can safely + // transform the Arc into an Rc and save Parser from needing atomic ref counting + // to manage its further references. + .get_or_init(|| { + let env_rc = unsafe { Rc::from_raw(&**EnvStack::principal() as *const _) }; + Parser::new(env_rc, true) + }) } /// Assert that this parser is allowed to execute on the current thread. @@ -753,8 +761,8 @@ pub fn vars(&self) -> &EnvStack { } /// Get the variables as an Arc. - pub fn vars_ref(&self) -> Arc { - Pin::into_inner(Pin::clone(&self.variables)) + pub fn vars_ref(&self) -> Rc { + Rc::clone(&self.variables) } /// Get the library data. diff --git a/src/tests/input.rs b/src/tests/input.rs index 0fadd4f39..e23972f72 100644 --- a/src/tests/input.rs +++ b/src/tests/input.rs @@ -4,14 +4,14 @@ use crate::parser::Parser; use crate::tests::prelude::*; use crate::wchar::prelude::*; -use std::sync::Arc; +use std::rc::Rc; #[test] #[serial] fn test_input() { let _cleanup = test_init(); use crate::env::EnvStack; - let parser = Parser::new(Arc::pin(EnvStack::new()), false); + let parser = Parser::new(Rc::new(EnvStack::new()), false); let mut input = Inputter::new(parser, libc::STDIN_FILENO); // Ensure sequences are order independent. Here we add two bindings where the first is a prefix // of the second, and then emit the second key list. The second binding should be invoked, not