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

Stop caching whether to lock

Browse Source 
The cached information might become outdated. It is important that all fish
processes use the same mutual exclusion logic (either `flock`-based or the
fallback), because the two methods do not provide mutual exclusion from one
another.
Avoiding caching makes the behavior independent on previous system states,
resulting in fish instances performing file operations at the same time to use
the same locking logic.

More detailed discussion in
https://github.com/fish-shell/fish-shell/pull/11492#discussion_r2134543447
This commit is contained in:
Daniel Rainer
2025-06-18 21:00:33 +02:00
parent 77738fd646
commit 8cbcfc0b3a
4 changed files with 44 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
src/env_universal_common.rs
View File

@@ -6,9 +6,7 @@
use crate::env::{EnvVar, EnvVarFlags, VarTable};
use crate::flog::{FLOG, FLOGF};
use crate::fs::{lock_and_load, rewrite_via_temporary_file, PotentialUpdate};
use crate::global_safety::RelaxedAtomicBool;
use crate::path::path_get_config;
use crate::path::{path_get_config_remoteness, DirRemoteness};
use crate::wchar::{decode_byte_from_char, prelude::*};
use crate::wcstringutil::{join_strings, LineIterator};
use crate::wutil::{file_id_for_file, file_id_for_path_narrow, wrealpath, FileId, INVALID_FILE_ID};
@@ -19,8 +17,6 @@
use std::io::{Read, Write};
use std::mem::MaybeUninit;
static LOCK_UVAR_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true);
/// Callback data, reflecting a change in universal variables.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct CallbackData {
@@ -146,11 +142,6 @@ 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) -> Option<CallbackDataList> {
// Set `LOCK_UVAR_FILE` to false immediately if the default variable path is on a remote filesystem.
// See #7968.
if path_get_config_remoteness() == DirRemoteness::remote {
LOCK_UVAR_FILE.store(false);
}
self.initialize_at_path(default_vars_path())
}
@@ -213,7 +204,7 @@ pub fn sync(&mut self) -> (bool, Option<CallbackDataList>) {
};
let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone());
match rewrite_via_temporary_file(&real_path, &LOCK_UVAR_FILE, rewrite) {
match rewrite_via_temporary_file(&real_path, rewrite) {
Ok((file_id, potential_update)) => {
self.last_read_file_id = file_id;
self.ok_to_save = potential_update.do_save;
@@ -379,7 +370,7 @@ fn load_from_path_narrow(&mut self) -> Option<CallbackDataList> {
}
FLOG!(uvar_file, "universal log reading from file");
match lock_and_load(&self.vars_path, &LOCK_UVAR_FILE, |f| {
match lock_and_load(&self.vars_path, |f| {
Ok(self.load_from_file(f).map(|update| update.data))
}) {
Ok((

95
src/fs.rs
View File

@@ -1,7 +1,6 @@
use crate::{
common::{str2wcstring, wcs2zstring, ScopeGuard},
fds::wopen_cloexec,
global_safety::RelaxedAtomicBool,
path::{path_remoteness, DirRemoteness},
wchar::prelude::*,
wutil::{
@@ -182,39 +181,29 @@ pub fn get_mut(&mut self) -> &mut File {
/// until it manages a run without the file being modified in the meantime, or until the maximum
/// number of allowed attempts is reached.
/// If the file does not exist this function will return an error.
pub fn lock_and_load<F, UserData>(
path: &wstr,
locking_enabled: &RelaxedAtomicBool,
load: F,
) -> std::io::Result<(FileId, UserData)>
pub fn lock_and_load<F, UserData>(path: &wstr, load: F) -> std::io::Result<(FileId, UserData)>
where
F: Fn(&File) -> std::io::Result<UserData>,
{
if locking_enabled.load() {
match LockedFile::new(LockingMode::Shared, path) {
Ok(locked_file) => {
return Ok((
file_id_for_file(locked_file.get()),
load(locked_file.get())?,
));
}
Err(e) => {
FLOGF!(
synced_file_access,
"Error acquiring shared lock on the directory of '%s': %s",
path,
e,
);
// This function might be called when the file does not exist.
// Do not abandon locking is such cases.
if e.kind() == std::io::ErrorKind::NotFound {
// There is no point in continuing in this function if the file does not
// exist.
return Err(e);
} else {
FLOG!(synced_file_access, "Disabling locking.");
locking_enabled.store(false);
}
match LockedFile::new(LockingMode::Shared, path) {
Ok(locked_file) => {
return Ok((
file_id_for_file(locked_file.get()),
load(locked_file.get())?,
));
}
Err(e) => {
FLOGF!(
synced_file_access,
"Error acquiring shared lock on the directory of '%s': %s",
path,
e,
);
// This function might be called when the file does not exist.
if e.kind() == std::io::ErrorKind::NotFound {
// There is no point in continuing in this function if the file does not
// exist.
return Err(e);
}
}
}
@@ -266,8 +255,6 @@ pub struct PotentialUpdate<UserData> {
/// # Arguments
///
/// - `path`: The path to the file which should be updated.
/// - `locking_enabled`: To indicate whether locking should be attempted. This function might
/// update the value stored in `locking_enabled`.
/// - `rewrite`: The function which handles reading from the file and writing to a temporary file.
/// The first argument is for the file to read from, the second for the temporary file to write to.
/// On success, the value returned by `rewrite` is included in this functions return value. Be
@@ -283,7 +270,6 @@ pub struct PotentialUpdate<UserData> {
/// but before we obtained the [`FileId`] from `path`. This is a race condition we do not detect.
pub fn rewrite_via_temporary_file<F, UserData>(
path: &wstr,
locking_enabled: &RelaxedAtomicBool,
rewrite: F,
) -> std::io::Result<(FileId, PotentialUpdate<UserData>)>
where
@@ -361,29 +347,26 @@ fn rename(old_name: &wstr, new_name: &wstr) -> std::io::Result<()> {
// To avoid issues with crashes during writing,
// we write to a temporary file and once we are done, this file is renamed such that it
// replaces the original file.
if locking_enabled.load() {
// To avoid races, we need to have exclusive access to the file for the entire
// duration, which we get via an exclusive lock on the parent directory.
// Taking a shared lock first and later upgrading to an exclusive one could result in a
// deadlock, so we take an exclusive one immediately.
match LockedFile::new(LockingMode::Exclusive(WriteMethod::RenameIntoPlace), path) {
Ok(locked_file) => {
let potential_update = rewrite(locked_file.get(), &mut tmp_file)?;
if potential_update.do_save {
update_metadata(locked_file.get(), &tmp_file);
rename(&tmp_name, path)?;
}
return Ok((file_id_for_path(path), potential_update));
}
Err(e) => {
FLOGF!(
synced_file_access,
"Error acquiring exclusive lock on the directory of '%s': %s",
path,
e,
);
locking_enabled.store(false);
// To avoid races, we need to have exclusive access to the file for the entire
// duration, which we get via an exclusive lock on the parent directory.
// Taking a shared lock first and later upgrading to an exclusive one could result in a
// deadlock, so we take an exclusive one immediately.
match LockedFile::new(LockingMode::Exclusive(WriteMethod::RenameIntoPlace), path) {
Ok(locked_file) => {
let potential_update = rewrite(locked_file.get(), &mut tmp_file)?;
if potential_update.do_save {
update_metadata(locked_file.get(), &tmp_file);
rename(&tmp_name, path)?;
}
return Ok((file_id_for_path(path), potential_update));
}
Err(e) => {
FLOGF!(
synced_file_access,
"Error acquiring exclusive lock on the directory of '%s': %s",
path,
e,
);
}
}

17
src/history.rs
View File

@@ -478,11 +478,7 @@ fn load_old_if_needed(&mut self) {
let _profiler = TimeProfiler::new("load_old");
if let Ok(Some(history_path)) = self.history_file_path() {
match lock_and_load(
&history_path,
&LOCK_HISTORY_FILE,
HistoryFileContents::create,
) {
match lock_and_load(&history_path, HistoryFileContents::create) {
Ok((file_id, file_contents)) => {
self.file_contents = Some(file_contents);
self.history_file_id = file_id;
@@ -647,7 +643,7 @@ fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> {
})
};
let (file_id, _) = rewrite_via_temporary_file(&history_path, &LOCK_HISTORY_FILE, rewrite)?;
let (file_id, _) = rewrite_via_temporary_file(&history_path, rewrite)?;
self.history_file_id = file_id;
// We've saved everything, so we have no more unsaved items.
@@ -669,13 +665,6 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> {
return Ok(());
};
if !LOCK_HISTORY_FILE.load() {
return Err(std::io::Error::new(
std::io::ErrorKind::Unsupported,
"Appending is not supported when locking is disabled.",
));
}
FLOGF!(
history,
"Saving %lu items via appending",
@@ -1841,5 +1830,3 @@ pub fn in_private_mode(vars: &dyn Environment) -> bool {
/// Whether to force the read path instead of mmap. This is useful for testing.
static NEVER_MMAP: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
static LOCK_HISTORY_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true);

9
src/history/file.rs
View File

@@ -141,14 +141,7 @@ pub fn create(mut history_file: &File) -> std::io::Result<Self> {
Err(err) => {
if err.raw_os_error() == Some(ENODEV) {
// Our mmap failed with ENODEV, which means the underlying
// filesystem does not support mapping. Treat this as a hint
// that the filesystem is remote, and so disable locks for
// the history file.
FLOG!(
history_file,
"Remote file system detected. Disabling history file locking.",
);
super::LOCK_HISTORY_FILE.store(false);
// filesystem does not support mapping.
// Create an anonymous mapping and read() the file into it.
map_anon(history_file, len)?
} else {