h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-06-06 00:41:15 -03:00
Code Issues Packages Projects Releases Wiki Activity

Return callbacks via return value

Browse Source 
Callbacks are generated by
`src/env_universal_common.rs:generate_callbacks_and_update_exports`.
This function only pushes to the `Vec`, so the content of the `Vec` passed to it
does not matter within this function.
The generated callbacks are used in `src/env/environment.rs:universal_sync`,
which calls `generate_callbacks_and_update_exports` via
`EnvUniversal::sync`,
(optionally `EnvUniversal::load_from_path_narrow`),
`EnvUniversal::load_from_file`.

The only other code making use of these callbacks is in tests.
Because the only real use passes an empty `Vec`, there is no need to to pass the
`Vec` as a mutable reference to the function at all. Instead, we create the `Vec`
in `generate_callbacks_and_update_exports` and return it from there, which is
what the new code does.

This change is made because we want loading from a file to be performed without
mutating data which does not come directly from the file.
Then, we can safely retry loading from the file as many times as we want,
without worrying about side-effects on our data structures.
We want to be able to do this in the case where we cannot properly lock the file
and fall back to lockless reading, where we check file metadata before and after
reading to detect modifications, and retry if modifications are detected.

This fallback logic is not in place yet, and further changes are required for
side-effect free loading.
This commit is contained in:
Daniel Rainer
2025-05-25 16:08:18 +02:00
parent 43fbfa9a0f
commit f40e31f675
3 changed files with 70 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
src/env/environment.rs vendored
View File

@@ -8,7 +8,6 @@
use crate::common::{str2wcstring, unescape_string, wcs2zstring, UnescapeStringStyle};
use crate::env::{EnvMode, EnvVar, Statuses};
use crate::env_dispatch::{env_dispatch_init, env_dispatch_var_change};
use crate::env_universal_common::CallbackDataList;
use crate::event::Event;
use crate::flog::FLOG;
use crate::global_safety::RelaxedAtomicBool;
@@ -351,22 +350,23 @@ pub fn universal_sync(&self, always: bool) -> Vec<Event> {
}
UVARS_LOCALLY_MODIFIED.store(false);
let mut callbacks = CallbackDataList::new();
let changed = uvars().sync(&mut callbacks);
let (changed, callbacks) = uvars().sync();
if changed {
default_notifier().post_notification();
}
// React internally to changes to special variables like LANG, and populate on-variable events.
let mut result = Vec::new();
for callback in callbacks {
let name = callback.key;
env_dispatch_var_change(&name, self);
let evt = if callback.val.is_none() {
Event::variable_erase(name)
} else {
Event::variable_set(name)
};
result.push(evt);
if let Some(callbacks) = callbacks {
for callback in callbacks {
let name = callback.key;
env_dispatch_var_change(&name, self);
let evt = if callback.val.is_none() {
Event::variable_erase(name)
} else {
Event::variable_set(name)
};
result.push(evt);
}
}
result
}
@@ -792,8 +792,7 @@ pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool
UVAR_SCOPE_IS_GLOBAL.store(true);
} else {
// Set up universal variables using the default path.
let mut callbacks = CallbackDataList::new();
uvars().initialize(&mut callbacks);
let callbacks = uvars().initialize().unwrap_or_default();
for callback in callbacks {
env_dispatch_var_change(&callback.key, vars);
}

58
src/env_universal_common.rs
View File

@@ -154,34 +154,32 @@ pub fn get_table(&self) -> &VarTable {
/// Initialize this uvars for the default path.
/// This should be called at most once on any given instance.
pub fn initialize(&mut self, callbacks: &mut CallbackDataList) {
pub fn initialize(&mut self) -> Option<CallbackDataList> {
// Set do_flock to false immediately if the default variable path is on a remote filesystem.
// See #7968.
if path_get_config_remoteness() == DirRemoteness::remote {
self.do_flock = false;
}
self.initialize_at_path(callbacks, default_vars_path());
self.initialize_at_path(default_vars_path())
}
/// Initialize a this uvars for a given path.
/// This is exposed for testing only.
pub fn initialize_at_path(&mut self, callbacks: &mut CallbackDataList, path: WString) {
pub fn initialize_at_path(&mut self, path: WString) -> Option<CallbackDataList> {
if path.is_empty() {
return;
return None;
}
assert!(!self.initialized(), "Already initialized");
self.vars_path = path;
if self.load_from_path(callbacks) {
// Successfully loaded from our normal path.
}
self.load_from_path()
}
/// Reads and writes variables at the correct path. Returns true if modified variables were
/// written.
pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool {
pub fn sync(&mut self) -> (bool, Option<CallbackDataList>) {
if !self.initialized() {
return false;
return (false, None);
}
FLOG!(uvar_file, "universal log sync");
@@ -216,9 +214,9 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool {
// with fire anyways.
// If we have no changes, just load.
if self.modified.is_empty() {
self.load_from_path_narrow(callbacks);
let callbacks = self.load_from_path_narrow();
FLOG!(uvar_file, "universal log no modifications");
return false;
return (false, callbacks);
}
let directory = wdirname(&self.vars_path).to_owned();
@@ -228,16 +226,16 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool {
// Open the file.
let Some(mut vars_file) = self.open_and_acquire_lock() else {
FLOG!(uvar_file, "universal log open_and_acquire_lock() failed");
return false;
return (false, None);
};
// Read from it.
self.load_from_file(&mut vars_file, callbacks);
let callbacks = self.load_from_file(&mut vars_file);
if self.ok_to_save {
self.save(&directory)
(self.save(&directory), callbacks)
} else {
true
(true, callbacks)
}
}
@@ -364,38 +362,38 @@ fn initialized(&self) -> bool {
!self.vars_path.is_empty()
}
fn load_from_path(&mut self, callbacks: &mut CallbackDataList) -> bool {
fn load_from_path(&mut self) -> Option<CallbackDataList> {
self.narrow_vars_path = wcs2zstring(&self.vars_path);
self.load_from_path_narrow(callbacks)
self.load_from_path_narrow()
}
fn load_from_path_narrow(&mut self, callbacks: &mut CallbackDataList) -> bool {
fn load_from_path_narrow(&mut self) -> Option<CallbackDataList> {
// Check to see if the file is unchanged. We do this again in load_from_file, but this avoids
// opening the file unnecessarily.
if self.last_read_file_id != INVALID_FILE_ID
&& file_id_for_path_narrow(&self.narrow_vars_path) == self.last_read_file_id
{
FLOG!(uvar_file, "universal log sync elided based on fast stat()");
return true;
return None;
}
let Ok(mut file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty())
else {
return false;
return None;
};
FLOG!(uvar_file, "universal log reading from file");
self.load_from_file(&mut file, callbacks);
true
self.load_from_file(&mut file)
}
// Load environment variables from the opened [`File`] `file`. It must be mutable because we
// will read from the underlying fd.
fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) {
fn load_from_file(&mut self, file: &mut File) -> Option<CallbackDataList> {
// Get the dev / inode.
let current_file_id = file_id_for_file(file);
if current_file_id == self.last_read_file_id {
FLOG!(uvar_file, "universal log sync elided based on fstat()");
None
} else {
// Read a variables table from the file.
let mut new_vars = VarTable::new();
@@ -408,11 +406,12 @@ fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList)
}
// Announce changes and update our exports generation.
self.generate_callbacks_and_update_exports(&new_vars, callbacks);
let callbacks = self.generate_callbacks_and_update_exports(&new_vars);
// Acquire the new variables.
self.acquire_variables(new_vars);
self.last_read_file_id = current_file_id;
Some(callbacks)
}
}
@@ -568,11 +567,8 @@ fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool {
// Given a variable table, generate callbacks representing the difference between our vars and
// the new vars. Also update our exports generation count as necessary.
fn generate_callbacks_and_update_exports(
&mut self,
new_vars: &VarTable,
callbacks: &mut CallbackDataList,
) {
fn generate_callbacks_and_update_exports(&mut self, new_vars: &VarTable) -> CallbackDataList {
let mut callbacks = CallbackDataList::new();
// Construct callbacks for erased values.
for (key, value) in &self.vars {
// Skip modified values.
@@ -616,6 +612,7 @@ fn generate_callbacks_and_update_exports(
});
}
}
callbacks
}
// Given a variable table, copy unmodified values into self.
@@ -628,8 +625,7 @@ fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) {
vars_to_acquire.remove(key);
}
Some(src) => {
// The value has been modified. Copy it over. Note we can destructively modify the
// source entry in vars since we are about to get rid of this->vars entirely.
// The value has been modified. Copy it over.
vars_to_acquire.insert(key.clone(), src.clone());
}
}

48
src/tests/env_universal_common.rs
View File

@@ -3,7 +3,7 @@
use crate::common::ScopeGuard;
use crate::common::ENCODE_DIRECT_BASE;
use crate::env::{EnvVar, EnvVarFlags, VarTable};
use crate::env_universal_common::{CallbackDataList, EnvUniversal, UvarFormat};
use crate::env_universal_common::{EnvUniversal, UvarFormat};
use crate::reader::{reader_pop, reader_push, ReaderConfig};
use crate::tests::prelude::*;
use crate::threads::{iothread_drain_all, iothread_perform};
@@ -15,15 +15,14 @@
fn test_universal_helper(x: usize) {
let _cleanup = test_init();
let mut callbacks = CallbackDataList::new();
let mut uvars = EnvUniversal::new();
uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
uvars.initialize_at_path(UVARS_TEST_PATH.to_owned());
for j in 0..UVARS_PER_THREAD {
let key = sprintf!("key_%d_%d", x, j);
let val = sprintf!("val_%d_%d", x, j);
uvars.set(&key, EnvVar::new(val, EnvVarFlags::empty()));
let synced = uvars.sync(&mut callbacks);
let (synced, _) = uvars.sync();
assert!(
synced,
"Failed to sync universal variables after modification"
@@ -32,7 +31,7 @@ fn test_universal_helper(x: usize) {
// Last step is to delete the first key.
uvars.remove(&sprintf!("key_%d_%d", x, 0));
let synced = uvars.sync(&mut callbacks);
let (synced, _) = uvars.sync();
assert!(synced, "Failed to sync universal variables after deletion");
}
@@ -54,8 +53,7 @@ fn test_universal() {
iothread_drain_all(&mut reader);
let mut uvars = EnvUniversal::new();
let mut callbacks = CallbackDataList::new();
uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
uvars.initialize_at_path(UVARS_TEST_PATH.to_owned());
for i in 0..threads {
for j in 0..UVARS_PER_THREAD {
@@ -219,11 +217,25 @@ fn test_universal_parsing_legacy() {
fn test_universal_callbacks() {
let _cleanup = test_init();
std::fs::create_dir_all("test/fish_uvars_test/").unwrap();
let mut callbacks = CallbackDataList::new();
let mut uvars1 = EnvUniversal::new();
let mut uvars2 = EnvUniversal::new();
uvars1.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
uvars2.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
let mut callbacks = uvars1
.initialize_at_path(UVARS_TEST_PATH.to_owned())
.unwrap_or_default();
callbacks.append(
&mut uvars2
.initialize_at_path(UVARS_TEST_PATH.to_owned())
.unwrap_or_default(),
);
macro_rules! sync {
($uvars:expr) => {
let (_, cb_opt) = $uvars.sync();
if let Some(mut cb) = cb_opt {
callbacks.append(&mut cb);
}
};
}
let noflags = EnvVarFlags::empty();
@@ -236,8 +248,8 @@ fn test_universal_callbacks() {
uvars1.set(L!("kappa"), EnvVar::new(L!("1").to_owned(), noflags)); //
uvars1.set(L!("omicron"), EnvVar::new(L!("1").to_owned(), noflags)); //
uvars1.sync(&mut callbacks);
uvars2.sync(&mut callbacks);
sync!(uvars1);
sync!(uvars2);
// Change uvars1.
uvars1.set(L!("alpha"), EnvVar::new(L!("2").to_owned(), noflags)); // changes value
@@ -247,7 +259,7 @@ fn test_universal_callbacks() {
); // changes export
uvars1.remove(L!("delta")); // erases value
uvars1.set(L!("epsilon"), EnvVar::new(L!("1").to_owned(), noflags)); // changes nothing
uvars1.sync(&mut callbacks);
sync!(uvars1);
// Change uvars2. It should treat its value as correct and ignore changes from uvars1.
uvars2.set(L!("lambda"), EnvVar::new(L!("1").to_owned(), noflags)); // same value
@@ -255,7 +267,7 @@ fn test_universal_callbacks() {
// Now see what uvars2 sees.
callbacks.clear();
uvars2.sync(&mut callbacks);
sync!(uvars2);
// Sort them to get them in a predictable order.
callbacks.sort_by(|a, b| a.key.cmp(&b.key));
@@ -308,12 +320,12 @@ fn test_universal_ok_to_save() {
"UVARS_TEST_PATH should be readable"
);
let mut cbs = CallbackDataList::new();
let mut uvars = EnvUniversal::new();
uvars.initialize_at_path(&mut cbs, UVARS_TEST_PATH.to_owned());
uvars
.initialize_at_path(UVARS_TEST_PATH.to_owned())
.unwrap_or_default();
assert!(!uvars.is_ok_to_save(), "Should not be OK to save");
uvars.sync(&mut cbs);
cbs.clear();
uvars.sync();
assert!(!uvars.is_ok_to_save(), "Should still not be OK to save");
uvars.set(
L!("SOMEVAR"),