diff --git a/po/de.po b/po/de.po index e54002510..693ae6bb9 100644 --- a/po/de.po +++ b/po/de.po @@ -1025,8 +1025,8 @@ msgid "Error reading script file '%s':" msgstr "Die Script-Datei '%s' kann nicht gelesen werden:" #, c-format -msgid "Error when renaming history file: %s" -msgstr "Fehler beim Umbenennen der Verlaufsdatei: %s" +msgid "Error when renaming file: %s" +msgstr "" #, c-format msgid "Error while reading file %ls\n" @@ -1249,14 +1249,6 @@ msgstr "" msgid "List or remove functions" msgstr "Funktionen auflisten oder entfernen" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "Sperren der Geschichts-Datei dauerte zu lang (%.3f Sekunden)." - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "Die Datei für universelle Variablen zu sperren dauerte zu lange (%.3f Sekunden)" - msgid "Logical operations are not supported, use `test` instead" msgstr "Logikoperationen werden nicht unterstützt, nimm stattdessen `test`" @@ -1519,6 +1511,10 @@ msgstr "Derzeit ausgewertete Funktion stoppen" msgid "Stop the innermost loop" msgstr "Innerste Schleife beenden" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Ereignisweitergabe vorübergehend blockieren" @@ -1623,22 +1619,10 @@ msgstr "Konnte Verzeichnis %ls abgeleitet von %ls nicht finden: '%ls'." msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "Die Universal-Variablen-Datei '%s' kann nicht geöffnet werden: %s" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "Konnte Datei '%ls' nicht zu '%ls' umbenennen: %s" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "Die Universal-Variablen-Datei '%ls' kann nicht geschrieben werden: %s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" @@ -79192,6 +79176,30 @@ msgstr "" msgid "~ expansion" msgstr "" +#, c-format +#~ msgid "Error when renaming history file: %s" +#~ msgstr "Fehler beim Umbenennen der Verlaufsdatei: %s" + +#, c-format +#~ msgid "Locking the history file took too long (%.3f seconds)." +#~ msgstr "Sperren der Geschichts-Datei dauerte zu lang (%.3f Sekunden)." + +#, c-format +#~ msgid "Locking the universal var file took too long (%.3f seconds)." +#~ msgstr "Die Datei für universelle Variablen zu sperren dauerte zu lange (%.3f Sekunden)" + +#, c-format +#~ msgid "Unable to open universal variable file '%s': %s" +#~ msgstr "Die Universal-Variablen-Datei '%s' kann nicht geöffnet werden: %s" + +#, c-format +#~ msgid "Unable to rename file from '%ls' to '%ls': %s" +#~ msgstr "Konnte Datei '%ls' nicht zu '%ls' umbenennen: %s" + +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "Die Universal-Variablen-Datei '%ls' kann nicht geschrieben werden: %s" + #~ msgid "builtin\n" #~ msgstr "Eingebauter Befehl\n" diff --git a/po/en.po b/po/en.po index 679c205e6..8430cf7c9 100644 --- a/po/en.po +++ b/po/en.po @@ -1023,7 +1023,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1246,14 +1246,6 @@ msgstr "" msgid "List or remove functions" msgstr "List or remove functions" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1516,6 +1508,10 @@ msgstr "Stop the currently evaluated function" msgid "Stop the innermost loop" msgstr "Stop the innermost loop" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Temporarily block delivery of events" @@ -1620,22 +1616,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "Unable to locate the %ls directory." -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "Unable to write to universal variables file '%ls': %s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" @@ -79188,6 +79172,10 @@ msgstr "" msgid "~ expansion" msgstr "" +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "Unable to write to universal variables file '%ls': %s" + #~ msgid "empty" #~ msgstr "empty" diff --git a/po/fr.po b/po/fr.po index b52fd2017..15e123669 100644 --- a/po/fr.po +++ b/po/fr.po @@ -1124,7 +1124,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1347,14 +1347,6 @@ msgstr "" msgid "List or remove functions" msgstr "Lister ou supprimer des fonctions" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "Le verrouillage du fichier d’historique a pris trop de temps (%.3f secondes)." - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "Le verrouillage du fichier des variables universel a pris trop de temps (%.3f secondes)." - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1617,6 +1609,10 @@ msgstr "Arrêter la fonction actuellement en évaluation" msgid "Stop the innermost loop" msgstr "Arrêter la boucle interne" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Bloquer temporairement la distribution des événements" @@ -1721,22 +1717,10 @@ msgstr "Impossible de localiser le dossier %ls obtenu de $%ls: '%ls'." msgid "Unable to locate the %ls directory." msgstr "Impossible de localiser le dossier %ls." -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "Impossible de renommer le fichier '%ls' en '%ls': %s" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "Impossible d’écrire dans le fichier de variables universelles '%ls': %s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" @@ -79289,6 +79273,22 @@ msgstr "" msgid "~ expansion" msgstr "" +#, c-format +#~ msgid "Locking the history file took too long (%.3f seconds)." +#~ msgstr "Le verrouillage du fichier d’historique a pris trop de temps (%.3f secondes)." + +#, c-format +#~ msgid "Locking the universal var file took too long (%.3f seconds)." +#~ msgstr "Le verrouillage du fichier des variables universel a pris trop de temps (%.3f secondes)." + +#, c-format +#~ msgid "Unable to rename file from '%ls' to '%ls': %s" +#~ msgstr "Impossible de renommer le fichier '%ls' en '%ls': %s" + +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "Impossible d’écrire dans le fichier de variables universelles '%ls': %s" + #~ msgid "Check that this terminal type is supported on this system." #~ msgstr "Vérifiez que votre type de terminal est supporté sur ce système" diff --git a/po/pl.po b/po/pl.po index bb9077060..f94fd17a3 100644 --- a/po/pl.po +++ b/po/pl.po @@ -1019,7 +1019,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1242,14 +1242,6 @@ msgstr "" msgid "List or remove functions" msgstr "Wypisz lub usuń funkcje" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1512,6 +1504,10 @@ msgstr "Zatrzymaj obecnie używaną funkcję" msgid "Stop the innermost loop" msgstr "" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "" @@ -1616,22 +1612,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index 605a79ada..4db94d587 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -1024,7 +1024,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1247,14 +1247,6 @@ msgstr "" msgid "List or remove functions" msgstr "Lista ou remove funções" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1517,6 +1509,10 @@ msgstr "Pára a função em execução" msgid "Stop the innermost loop" msgstr "Pára o laço mais interno" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Bloqueia temporariamente a entrega de eventos" @@ -1621,22 +1617,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" diff --git a/po/sv.po b/po/sv.po index 2387b9804..d2feb7704 100644 --- a/po/sv.po +++ b/po/sv.po @@ -1020,7 +1020,7 @@ msgid "Error reading script file '%s':" msgstr "" #, c-format -msgid "Error when renaming history file: %s" +msgid "Error when renaming file: %s" msgstr "" #, c-format @@ -1243,14 +1243,6 @@ msgstr "" msgid "List or remove functions" msgstr "Visa eller ta bort funktioner" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "" - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "" - msgid "Logical operations are not supported, use `test` instead" msgstr "" @@ -1513,6 +1505,10 @@ msgstr "Avbryt den nuvarande funktionen" msgid "Stop the innermost loop" msgstr "Avbryt den innersta loopen" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Blockera tillfälligt leverans av händelser" @@ -1617,22 +1613,10 @@ msgstr "" msgid "Unable to locate the %ls directory." msgstr "" -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "" - #, c-format msgid "Unable to read input file: %s" msgstr "" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "" - msgid "Unexpected ')' for unopened parenthesis" msgstr "" diff --git a/po/zh_CN.po b/po/zh_CN.po index 29cdcad93..1e2a97b4d 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -1017,8 +1017,8 @@ msgid "Error reading script file '%s':" msgstr "读取脚本文件 '%s'出错: " #, c-format -msgid "Error when renaming history file: %s" -msgstr "重命名历史文件时出错:%s" +msgid "Error when renaming file: %s" +msgstr "" #, c-format msgid "Error while reading file %ls\n" @@ -1241,14 +1241,6 @@ msgstr "" msgid "List or remove functions" msgstr "列出或移除函数" -#, c-format -msgid "Locking the history file took too long (%.3f seconds)." -msgstr "锁定历史文件花费了太长时间(%.3f 秒)." - -#, c-format -msgid "Locking the universal var file took too long (%.3f seconds)." -msgstr "锁定 通用 var 文件花费了太长时间 (%.3f 秒) ." - msgid "Logical operations are not supported, use `test` instead" msgstr "不支持逻辑操作,使用`test`替代" @@ -1511,6 +1503,10 @@ msgstr "停止当前求值的函数" msgid "Stop the innermost loop" msgstr "停止最内层的循环" +# +msgid "Synchronized file access" +msgstr "" + msgid "Temporarily block delivery of events" msgstr "Temporarily block delivery of events" @@ -1615,22 +1611,10 @@ msgstr "无法定位来自 $ls 的 %ls 目录: %ls ." msgid "Unable to locate the %ls directory." msgstr "无法找到 %ls 目录 ." -#, c-format -msgid "Unable to open universal variable file '%s': %s" -msgstr "无法打开通用变量文件'%s': %s" - #, c-format msgid "Unable to read input file: %s" msgstr "无法读取输入文件:%s" -#, c-format -msgid "Unable to rename file from '%ls' to '%ls': %s" -msgstr "无法将文件从 '%ls' 重命名为 '%ls':%s" - -#, c-format -msgid "Unable to write to universal variables file '%ls': %s" -msgstr "无法写入通用变量文件 '%ls':%s" - msgid "Unexpected ')' for unopened parenthesis" msgstr "未打开的括号' ' " @@ -79187,6 +79171,30 @@ msgstr "缩放" msgid "~ expansion" msgstr "~ 扩大" +#, c-format +#~ msgid "Error when renaming history file: %s" +#~ msgstr "重命名历史文件时出错:%s" + +#, c-format +#~ msgid "Locking the history file took too long (%.3f seconds)." +#~ msgstr "锁定历史文件花费了太长时间(%.3f 秒)." + +#, c-format +#~ msgid "Locking the universal var file took too long (%.3f seconds)." +#~ msgstr "锁定 通用 var 文件花费了太长时间 (%.3f 秒) ." + +#, c-format +#~ msgid "Unable to open universal variable file '%s': %s" +#~ msgstr "无法打开通用变量文件'%s': %s" + +#, c-format +#~ msgid "Unable to rename file from '%ls' to '%ls': %s" +#~ msgstr "无法将文件从 '%ls' 重命名为 '%ls':%s" + +#, c-format +#~ msgid "Unable to write to universal variables file '%ls': %s" +#~ msgstr "无法写入通用变量文件 '%ls':%s" + #~ msgid "builtin\n" #~ msgstr "内建\n" diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index b47116f99..c9e5e2628 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -1,40 +1,28 @@ #![allow(clippy::bad_bit_mask)] use crate::common::{ - timef, unescape_string, valid_var_name, wcs2zstring, UnescapeFlags, UnescapeStringStyle, + unescape_string, valid_var_name, wcs2zstring, UnescapeFlags, UnescapeStringStyle, }; use crate::env::{EnvVar, EnvVarFlags, VarTable}; -use crate::fds::{open_cloexec, wopen_cloexec}; use crate::flog::{FLOG, FLOGF}; -use crate::fs::create_temporary_file; +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, file_id_for_path_narrow, wdirname, wrealpath, wrename, - wstat, wunlink, FileId, INVALID_FILE_ID, -}; -use errno::{errno, Errno}; -use libc::{EINTR, LOCK_EX}; -use nix::{fcntl::OFlag, sys::stat::Mode}; +use crate::wutil::{file_id_for_file, file_id_for_path_narrow, wrealpath, FileId, INVALID_FILE_ID}; use std::collections::hash_map::Entry; use std::collections::HashSet; use std::ffi::CString; use std::fs::File; use std::io::{Read, Write}; use std::mem::MaybeUninit; -use std::os::fd::AsRawFd; -use std::os::unix::prelude::MetadataExt; -// Pull in the O_EXLOCK constant if it is defined, otherwise set it to 0. -#[cfg(any(apple, bsd))] -const O_EXLOCK: OFlag = OFlag::O_EXLOCK; - -#[cfg(not(any(apple, bsd)))] -const O_EXLOCK: OFlag = OFlag::empty(); +static LOCK_UVAR_FILE: RelaxedAtomicBool = RelaxedAtomicBool::new(true); /// Callback data, reflecting a change in universal variables. +#[derive(Clone, Debug, Eq, PartialEq)] pub struct CallbackData { // The name of the variable. pub key: WString, @@ -72,13 +60,11 @@ pub struct EnvUniversal { // Whether it's OK to save. This may be set to false if we discover that a future version of // fish wrote the uvars contents. + // Only update if last_read_file_id is updated as well. ok_to_save: bool, - // If true, attempt to flock the uvars file. - // This latches to false if the file is found to be remote, where flock may hang. - do_flock: bool, - // File id from which we last read. + // Only update if ok_to_save is updated as well. last_read_file_id: FileId, } @@ -99,7 +85,6 @@ pub fn new() -> Self { modified: Default::default(), export_generation: 1, ok_to_save: true, - do_flock: true, last_read_file_id: INVALID_FILE_ID, } } @@ -161,10 +146,10 @@ 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 { - // Set do_flock to false immediately if the default variable path is on a remote filesystem. + // 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 { - self.do_flock = false; + LOCK_UVAR_FILE.store(false); } self.initialize_at_path(default_vars_path()) } @@ -181,43 +166,16 @@ pub fn initialize_at_path(&mut self, path: WString) -> Option self.load_from_path() } - /// Reads and writes variables at the correct path. Returns true if modified variables were - /// written. + /// Reads and writes variables at the correct path. + /// If there is an existing variables file with an unknown format, parsing it is attempted, + /// but it will not be overwritten. + /// Returns whether data was read, and the callbacks. pub fn sync(&mut self) -> (bool, Option) { if !self.initialized() { return (false, None); } FLOG!(uvar_file, "universal log sync"); - // Our saving strategy: - // - // 1. Open the file, producing an fd. - // 2. Lock the file (may be combined with step 1 on systems with O_EXLOCK) - // 3. After taking the lock, check if the file at the given path is different from what we - // opened. If so, start over. - // 4. Read from the file. This can be elided if its dev/inode is unchanged since the last read - // 5. Open an adjacent temporary file - // 6. Write our changes to an adjacent file - // 7. Move the adjacent file into place via rename. This is assumed to be atomic. - // 8. Release the lock and close the file - // - // Consider what happens if Process 1 and 2 both do this simultaneously. Can there be data loss? - // Process 1 opens the file and then attempts to take the lock. Now, either process 1 will see - // the original file, or process 2's new file. If it sees the new file, we're OK: it's going to - // read from the new file, and so there's no data loss. If it sees the old file, then process 2 - // must have locked it (if process 1 locks it, switch their roles). The lock will block until - // process 2 reaches step 7; at that point process 1 will reach step 2, notice that the file has - // changed, and then start over. - // - // It's possible that the underlying filesystem does not support locks (lockless NFS). In this - // case, we risk data loss if two shells try to write their universal variables simultaneously. - // In practice this is unlikely, since uvars are usually written interactively. - // - // Prior versions of fish used a hard link scheme to support file locking on lockless NFS. The - // risk here is that if the process crashes or is killed while holding the lock, future - // instances of fish will not be able to obtain it. This seems to be a greater risk than that of - // data loss on lockless NFS. Users who put their home directory on lockless NFS are playing - // with fire anyways. // If we have no changes, just load. if self.modified.is_empty() { let callbacks = self.load_from_path_narrow(); @@ -225,40 +183,60 @@ pub fn sync(&mut self) -> (bool, Option) { return (false, callbacks); } - let directory = wdirname(&self.vars_path).to_owned(); - FLOG!(uvar_file, "universal log performing full sync"); - // Open the file. - let Some(vars_file) = self.open_and_acquire_lock() else { - FLOG!(uvar_file, "universal log open_and_acquire_lock() failed"); - return (false, None); - }; - - // TODO: proper locking - match self.load_from_file(&vars_file) { - Some(UniversalReadUpdate { - export_generation_increment, - new_vars, - callbacks, - ok_to_save, - }) => { - self.export_generation += export_generation_increment; - self.vars = new_vars; - self.ok_to_save = ok_to_save; - if self.ok_to_save { - (self.save(&directory), Some(callbacks)) - } else { - (true, Some(callbacks)) + let rewrite = |old_file: &File, + tmp_file: &mut File| + -> std::io::Result>> { + match self.load_from_file(old_file) { + Some(potential_update) => { + if potential_update.do_save { + let contents = Self::serialize_with_vars(&potential_update.data.new_vars); + tmp_file.write_all(&contents)?; + } + Ok(PotentialUpdate { + do_save: potential_update.do_save, + data: Some(potential_update.data), + }) + } + None => { + if self.ok_to_save { + let contents = Self::serialize_with_vars(&self.vars); + tmp_file.write_all(&contents)?; + } + Ok(PotentialUpdate { + do_save: self.ok_to_save, + data: None, + }) } } - None => { - if self.ok_to_save { - (self.save(&directory), None) - } else { - (true, None) + }; + + 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) { + Ok((file_id, potential_update)) => { + self.last_read_file_id = file_id; + self.ok_to_save = potential_update.do_save; + self.modified.clear(); + match potential_update.data { + Some(UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save, + }) => { + assert_eq!(potential_update.do_save, ok_to_save); + self.export_generation += export_generation_increment; + self.vars = new_vars; + (true, Some(callbacks)) + } + None => (true, None), } } + Err(e) => { + FLOG!(uvar_file, "universal log sync failed:", e); + (false, None) + } } } @@ -400,32 +378,46 @@ fn load_from_path_narrow(&mut self) -> Option { return None; } - let Ok(file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { - return None; - }; - FLOG!(uvar_file, "universal log reading from file"); - // TODO: proper locking - match self.load_from_file(&file) { - Some(UniversalReadUpdate { - export_generation_increment, - new_vars, - callbacks, - ok_to_save, - }) => { + match lock_and_load(&self.vars_path, &LOCK_UVAR_FILE, |f| { + Ok(self.load_from_file(f).map(|update| update.data)) + }) { + Ok(( + file_id, + Some(UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save, + }), + )) => { self.export_generation += export_generation_increment; self.vars = new_vars; self.ok_to_save = ok_to_save; + self.last_read_file_id = file_id; Some(callbacks) } - None => None, + Ok((_, None)) => { + // WARNING: Do not update self.file_id here, as this would interfere with + // self.ok_to_save. + None + } + Err(e) => { + FLOG!(uvar_file, "Failed to load from universal variable file:", e); + None + } } } - /// Load environment variables from the opened [`file`](`File`). - /// If successful, [`None`] indicates that no updates were necessary, whereas the [`Some`] - /// variant contains updated data. - fn load_from_file(&mut self, file: &File) -> Option { + /// Load environment variables from the [`file`](`File`). + /// If the file has not been modified since the last read, [`None`] is returned. + /// In this case, `self.ok_to_save` needs to be consulted to decide whether to rewrite the file. + /// Otherwise, the [`Some`] variant contains updated data and indicates whether the file should + /// be rewritten.. + // IMPORTANT: Callers of this code assume that a return value of None means that the file id has + // not changed. Do not return None in other situations without modifying the callers + // accordingly. Otherwise, problems with self.ok_to_save are expected to occur. + fn load_from_file(&self, file: &File) -> Option> { // Get the dev / inode. let current_file_id = file_id_for_file(file); if current_file_id == self.last_read_file_id { @@ -438,7 +430,7 @@ fn load_from_file(&mut self, file: &File) -> Option { // Hacky: if the read format is in the future, avoid overwriting the file: never try to // save. - let ok_to_save = format != UvarFormat::future; + let do_save = format != UvarFormat::future; // Announce changes and update our exports generation. let (export_generation_increment, callbacks) = @@ -446,126 +438,18 @@ fn load_from_file(&mut self, file: &File) -> Option { // Acquire the new variables. self.acquire_variables(&mut new_vars); - self.last_read_file_id = current_file_id; - Some(UniversalReadUpdate { - export_generation_increment, - new_vars, - callbacks, - ok_to_save, + Some(PotentialUpdate { + do_save, + data: UniversalReadUpdate { + export_generation_increment, + new_vars, + callbacks, + ok_to_save: do_save, + }, }) } } - // Functions concerned with saving. - fn open_and_acquire_lock(&mut self) -> Option { - // Attempt to open the file for reading at the given path, atomically acquiring a lock. On BSD, - // we can use O_EXLOCK. On Linux, we open the file, take a lock, and then compare fstat() to - // stat(); if they match, it means that the file was not replaced before we acquired the lock. - // - // We pass O_RDONLY with O_CREAT; this creates a potentially empty file. We do this so that we - // have something to lock on. - let mut locked_by_open = false; - let mut flags = OFlag::O_RDWR | OFlag::O_CREAT; - - if !O_EXLOCK.is_empty() && self.do_flock { - flags |= O_EXLOCK; - locked_by_open = true; - } - - loop { - let mut file = - match wopen_cloexec(&self.vars_path, flags, Mode::from_bits_truncate(0o644)) { - Ok(file) => file, - Err(nix::Error::EINTR) => continue, - Err(err) => { - if !O_EXLOCK.is_empty() { - if flags.contains(O_EXLOCK) - && [nix::Error::ENOTSUP, nix::Error::EOPNOTSUPP].contains(&err) - { - // Filesystem probably does not support locking. Give up on locking. - // Note that on Linux the two errno symbols have the same value but on BSD they're - // different. - flags &= !O_EXLOCK; - self.do_flock = false; - locked_by_open = false; - continue; - } - } - FLOG!( - error, - wgettext_fmt!( - "Unable to open universal variable file '%s': %s", - &self.vars_path, - err.to_string() - ) - ); - return None; - } - }; - - // Lock if we want to lock and open() didn't do it for us. - // If flock fails, give up on locking forever. - if self.do_flock && !locked_by_open { - if !flock_uvar_file(&mut file) { - self.do_flock = false; - } - } - - // Hopefully we got the lock. However, it's possible the file changed out from under us - // while we were waiting for the lock. Make sure that didn't happen. - if file_id_for_file(&file) != file_id_for_path(&self.vars_path) { - // Oops, it changed! Try again. - drop(file); - } else { - return Some(file); - } - } - } - - /// Writes our state to the [`file`](`File`). path is provided only for error reporting. - fn write_to_file(&mut self, file: &mut File, path: &wstr) -> std::io::Result<()> { - let contents = Self::serialize_with_vars(&self.vars); - - let res = file.write_all(&contents); - match res.as_ref() { - Ok(()) => { - // Since we just wrote out this file, it matches our internal state; pretend we read from it. - self.last_read_file_id = file_id_for_file(file); - } - Err(err) => { - let error = Errno(err.raw_os_error().unwrap()); - FLOG!( - error, - wgettext_fmt!( - "Unable to write to universal variables file '%ls': %s", - path, - error.to_string() - ), - ); - } - } - - // We don't close the file. - res - } - - fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool { - let ret = wrename(src, dst); - if ret != 0 { - let error = errno(); - FLOG!( - error, - wgettext_fmt!( - "Unable to rename file from '%ls' to '%ls': %s", - src, - dst, - error.to_string() - ) - ); - } - ret == 0 - } - /// Given a variable table, generate callbacks representing the difference between our vars and /// the new vars. /// Returns by how much the exports generation count should be incremented, as well as a @@ -746,85 +630,6 @@ fn read_message_internal(file: &File, vars: &mut VarTable) -> UvarFormat { Self::populate_variables(&contents, vars) } - - // Write our file contents. - // Return true on success, false on failure. - fn save(&mut self, directory: &wstr) -> bool { - use crate::common::ScopeGuard; - assert!(self.ok_to_save, "It's not OK to save"); - - // Open adjacent temporary file. - let tmp_name_template = directory.to_owned() + L!("/fishd.tmp.XXXXXX"); - let (mut private_file, private_file_path) = match create_temporary_file(&tmp_name_template) - { - Ok(tmp_file_data) => tmp_file_data, - Err(e) => { - FLOG!(warning, "Could not create temporary file:", e); - return false; - } - }; - // unlink pfp upon failure. In case of success, it (already) won't exist. - let delete_pfp = ScopeGuard::new(private_file_path, |path| { - wunlink(&path); - }); - let private_file_path = &delete_pfp; - - // Write to it. - if let Err(e) = self.write_to_file(&mut private_file, private_file_path) { - FLOG!(uvar_file, "universal log write_to_file() failed:", e); - return false; - } - - let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone()); - - // Ensure we maintain ownership and permissions (#2176). - if let Ok(md) = wstat(&real_path) { - // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73 - if unsafe { libc::fchown(private_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { - FLOG!(uvar_file, "universal log fchown() failed:", errno()); - } - if let Err(e) = private_file.set_permissions(md.permissions()) { - FLOG!(uvar_file, "universal log setting permissions failed:", e); - } - } - - // Linux by default stores the mtime with low precision, low enough that updates that occur - // in quick succession may result in the same mtime (even the nanoseconds field). So - // manually set the mtime of the new file to a high-precision clock. Note that this is only - // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other - // platforms we tend to notice the file has changed due to a different inode (or file size!) - // - // The current time within the Linux kernel is cached, and generally only updated on a timer - // interrupt. So if the timer interrupt is running at 10 milliseconds, the cached time will - // only be updated once every 10 milliseconds. - // - // It's probably worth finding a simpler solution to this. The tests ran into this, but it's - // unlikely to affect users. - #[cfg(any(target_os = "linux", target_os = "android"))] - { - let mut times: [libc::timespec; 2] = unsafe { std::mem::zeroed() }; - times[0].tv_nsec = libc::UTIME_OMIT; // don't change ctime - if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } == 0 { - unsafe { - libc::futimens(private_file.as_raw_fd(), ×[0]); - } - } - } - - // Apply new file. - if !self.move_new_vars_file_into_place(private_file_path, &real_path) { - FLOG!( - uvar_file, - "universal log move_new_vars_file_into_place() failed" - ); - return false; - } - - // Success at last. All of our modified variables have now been written out. - self.modified.clear(); - ScopeGuard::cancel(delete_pfp); - true - } } /// Return the default variable path, or an empty string on failure. @@ -1005,29 +810,6 @@ fn encode_serialized(vals: &[WString]) -> WString { join_strings(vals, UVAR_ARRAY_SEP) } -/// Try locking the file. -/// Return true on success, false on error. -fn flock_uvar_file(file: &mut File) -> bool { - let start_time = timef(); - while unsafe { libc::flock(file.as_raw_fd(), LOCK_EX) } == -1 { - if errno().0 != EINTR { - return false; // do nothing per issue #2149 - } - } - let duration = timef() - start_time; - if duration > 0.25 { - FLOG!( - warning, - wgettext_fmt!( - "Locking the universal var file took too long (%.3f seconds).", - duration - ) - ); - return false; - } - true -} - fn skip_spaces(mut s: &wstr) -> &wstr { while s.starts_with(L!(" ")) || s.starts_with(L!("\t")) { s = &s[1..]; diff --git a/src/flog.rs b/src/flog.rs index b3fdb203b..f9276c283 100644 --- a/src/flog.rs +++ b/src/flog.rs @@ -124,6 +124,8 @@ pub fn all_categories() -> Vec<&'static category_t> { (profile_history, "profile-history", "History performance measurements"); + (synced_file_access, "synced-file-access", "Synchronized file access"); + (iothread, "iothread", "Background IO thread events"); (fd_monitor, "fd-monitor", "FD monitor events"); diff --git a/src/fs.rs b/src/fs.rs index 3e926f510..28afdb873 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1,10 +1,26 @@ use crate::{ - common::{str2wcstring, wcs2zstring}, + common::{str2wcstring, wcs2zstring, ScopeGuard}, + fds::wopen_cloexec, + global_safety::RelaxedAtomicBool, + path::{path_remoteness, DirRemoteness}, wchar::prelude::*, - FLOG, + wutil::{ + file_id_for_file, file_id_for_path, wdirname, wrename, wunlink, FileId, INVALID_FILE_ID, + }, + FLOG, FLOGF, }; use errno::errno; -use std::{ffi::CString, fs::File, os::fd::FromRawFd}; +use libc::{c_int, fchown, flock, LOCK_EX, LOCK_SH}; +use nix::{fcntl::OFlag, sys::stat::Mode}; +use std::{ + ffi::CString, + fs::File, + io::Seek, + os::{ + fd::{AsRawFd, FromRawFd}, + unix::fs::MetadataExt, + }, +}; // Replacement for mkostemp(str, O_CLOEXEC) // This uses mkostemp if available, @@ -56,3 +72,378 @@ pub fn create_temporary_file(name_template: &wstr) -> std::io::Result<(File, WSt }; Ok((fd, str2wcstring(c_string_template.to_bytes()))) } + +/// Use this struct for all accesses to file which need mutual exclusion. +/// Otherwise, races on the file are possible. +/// The lock is released when this struct is dropped. +pub struct LockedFile { + /// This is the file which requires mutual exclusion. + /// It should only be accessed through this struct, + /// because the locks used here do not protect from other accesses to the file. + data_file: File, + /// The file descriptor of the parent directory, used for locking. + /// The lock is not placed on the data file directly due to issues with renaming. + /// If the data file is renamed after opening and before locking it, + /// There are two independent files around, whose locks do not interact. + /// In some cases this can be identified by checking file identifiers and timestamps, + /// but even with such checks races and corresponding file corruption can occur. + /// It is simpler to lock a different path, which does not change. + /// + /// We may fail to lock (e.g. on lockless NFS - see issue #685. + /// In that case, we proceed as if locking succeeded. + /// This might result in corruption, + /// but the alternative of not being able to access the file at all is not desirable either. + _locked_fd: File, +} + +pub const LOCKED_FILE_MODE: Mode = Mode::from_bits_truncate(0o600); + +pub enum LockingMode { + Shared, + Exclusive(WriteMethod), +} +pub enum WriteMethod { + Append, + RenameIntoPlace, +} + +impl LockingMode { + pub fn flock_op(&self) -> c_int { + match self { + Self::Shared => LOCK_SH, + Self::Exclusive(_) => LOCK_EX, + } + } + + pub fn file_flags(&self) -> OFlag { + match self { + Self::Shared => OFlag::O_RDONLY, + Self::Exclusive(WriteMethod::Append) => { + OFlag::O_WRONLY | OFlag::O_APPEND | OFlag::O_CREAT + } + Self::Exclusive(WriteMethod::RenameIntoPlace) => OFlag::O_RDONLY | OFlag::O_CREAT, + } + } +} + +impl LockedFile { + /// Creates a [`LockedFile`]. + /// Use this for any access to a which requires mutual exclusion, as it ensures correct locking. + /// Use [`LockingMode::Exclusive`] if you want to modify the file in any way. + /// Otherwise you should use [`LockingMode::Shared`]. + /// Two modes of modification are supported: + /// - Appending + /// - Writing to a temporary file which is then renamed into place. + /// File flags are derived from the [`LockingMode`]. + /// `file_name` should just be a name, not a full path. + pub fn new(locking_mode: LockingMode, file_path: &wstr) -> std::io::Result { + let dir_path = wdirname(file_path); + + if path_remoteness(dir_path) == DirRemoteness::remote { + return Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "Directory considered remote. Locking is disabled on remote file systems.", + )); + } + + // We start by locking the directory. + // This is required to avoid racing modifications by other threads/processes. + let dir_fd = wopen_cloexec(dir_path, OFlag::O_RDONLY, Mode::empty())?; + + // Try locking the directory. Retry if locking was interrupted. + while unsafe { flock(dir_fd.as_raw_fd(), locking_mode.flock_op()) } == -1 { + let err = std::io::Error::last_os_error(); + if err.kind() != std::io::ErrorKind::Interrupted { + return Err(err); + } + } + + // Open the data file + let data_file = wopen_cloexec(file_path, locking_mode.file_flags(), LOCKED_FILE_MODE)?; + + Ok(Self { + data_file, + _locked_fd: dir_fd, + }) + } + + pub fn get(&self) -> &File { + &self.data_file + } + + pub fn get_mut(&mut self) -> &mut File { + &mut self.data_file + } +} + +/// Runs the `load` function, which should see a consistent state of the file at `path`. +/// To ensure a consistent state, we use locks to prevent modifications by others. +/// If locking is unavailable, the `load` function might be executed multiple times, +/// 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( + path: &wstr, + locking_enabled: &RelaxedAtomicBool, + load: F, +) -> std::io::Result<(FileId, UserData)> +where + F: Fn(&File) -> std::io::Result, +{ + 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); + } + } + } + } + + FLOG!( + synced_file_access, + "flock-based locking is disabled. Using fallback implementation." + ); + + // Fallback implementation for situations where locking is unavailable. + let max_attempts = 1000; + for _ in 0..max_attempts { + let initial_file_id = file_id_for_path(path); + // If we cannot open the file, there is nothing we can do, + // so just return immediately. + let file = wopen_cloexec(path, OFlag::O_RDONLY, Mode::empty())?; + let loaded_data = match load(&file) { + Ok(update_data) => update_data, + Err(_) => { + // Retry if load function failed. Because we do not hold a lock, this might be + // caused by concurrent modifications. + continue; + } + }; + + let final_file_id = file_id_for_path(path); + if initial_file_id != final_file_id { + continue; + } + // If the file id did not change, we assume that we loaded a consistent state. + return Ok((final_file_id, loaded_data)); + } + Err(std::io::Error::new(std::io::ErrorKind::Other, "Failed to update the file. Locking is disabled, and the fallback code did not succeed within the permissible number of attempts.")) +} + +pub struct PotentialUpdate { + pub do_save: bool, + pub data: UserData, +} + +/// Use this function for updating a file based on its current content, +/// for files which might be accessed at the same time by other threads/processes. +/// The basic principle is to create a temporary file, take a lock on the file to be rewritten, +/// do the rewrite (which might involve reading the old file contents), +/// and finally release the lock. +/// Error handling (especially the case where locking is not possible) makes the implementation +/// non-straightforward. +/// +/// # 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 +/// careful about side effects of `rewrite`. It might get executed multiple times. Try to avoid +/// side effects and instead extract any data you might need and return them on success. +/// Then, apply the desired side effects once this function has returned successfully. +/// +/// # Return value +/// +/// On success, the [`FileId`] of the rewritten file is returned, alongside the value returned by +/// `rewrite`. Note that if locking is unavailable, the [`FileId`] might be the id of a different +/// version of the file, which was written after this function renamed the temporary file to `path` +/// but before we obtained the [`FileId`] from `path`. This is a race condition we do not detect. +pub fn rewrite_via_temporary_file( + path: &wstr, + locking_enabled: &RelaxedAtomicBool, + rewrite: F, +) -> std::io::Result<(FileId, PotentialUpdate)> +where + F: Fn(&File, &mut File) -> std::io::Result>, +{ + /// Updates the metadata of the `new_file` to match the `old_file`. + /// Also updates the mtime of the `new_file` to the current time manually, to work around + /// operating systems which do not update the internal time used for such time stamps + /// frequently enough. This is important for [`FileId`] comparisons. + fn update_metadata(old_file: &File, new_file: &File) { + // Ensure we maintain the ownership and permissions of the original (#2355). If the + // stat fails, we assume (hope) our default permissions are correct. This + // corresponds to e.g. someone running sudo -E as the very first command. If they + // did, it would be tricky to set the permissions correctly. (bash doesn't get this + // case right either). + if let Ok(md) = old_file.metadata() { + // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73 + if unsafe { fchown(new_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { + FLOG!( + synced_file_access, + "Error when changing ownership of file:", + errno::errno() + ); + } + if let Err(e) = new_file.set_permissions(md.permissions()) { + FLOG!(synced_file_access, "Error when changing mode of file:", e); + } + } else { + FLOG!(synced_file_access, "Could not get metadata for file"); + } + // Linux by default stores the mtime with low precision, low enough that updates that occur + // in quick succession may result in the same mtime (even the nanoseconds field). So + // manually set the mtime of the new file to a high-precision clock. Note that this is only + // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other + // platforms we tend to notice the file has changed due to a different inode. + // + // The current time within the Linux kernel is cached, and generally only updated on a timer + // interrupt. So if the timer interrupt is running at 10 milliseconds, the cached time will + // only be updated once every 10 milliseconds. + #[cfg(any(target_os = "linux", target_os = "android"))] + { + let mut times: [libc::timespec; 2] = unsafe { std::mem::zeroed() }; + times[0].tv_nsec = libc::UTIME_OMIT; // don't change atime + if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } == 0 { + unsafe { + // This accesses both times[0] and times[1]. Check `utimensat(2)` for details. + libc::futimens(new_file.as_raw_fd(), ×[0]); + } + } + } + } + + /// Renames a file from `old_name` to `new_name`. + fn rename(old_name: &wstr, new_name: &wstr) -> std::io::Result<()> { + if wrename(old_name, new_name) == -1 { + let error_number = errno::errno(); + FLOG!( + error, + wgettext_fmt!("Error when renaming file: %s", error_number.to_string()) + ); + return Err(std::io::Error::from(error_number)); + } + Ok(()) + } + + const TMP_FILE_SUFFIX: &wstr = L!(".XXXXXX"); + let tmp_file_template = path.to_owned() + TMP_FILE_SUFFIX; + let (tmp_file, tmp_name) = create_temporary_file(&tmp_file_template)?; + // Ensure that the temporary file is unlinked when this function returns. + let mut tmp_file = ScopeGuard::new(tmp_file, |_| { + wunlink(&tmp_name); + }); + + // We want to rewrite the file. + // 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); + } + } + } + + // If this is reached, we assume that locking is not available so we use a fallback + // implementation which tries to avoid race conditions, but in the case of contention it is + // possible that some writes are lost. + + FLOG!( + synced_file_access, + "flock-based locking is disabled. Using fallback implementation." + ); + + // Give up after this many unsuccessful attempts. + // TODO: which value, should this be a constant shared with other retrying logic? + let max_attempts = 1000; + for _ in 0..max_attempts { + // Truncate tmp_file for the next attempt to get rid of data potentially written in the + // previous iteration. + tmp_file.set_len(0)?; + tmp_file.seek(std::io::SeekFrom::Start(0)).unwrap(); + + // If the file does not exist yet, this will be `INVALID_FILE_ID`. + let initial_file_id = file_id_for_path(path); + // If we cannot open the file, there is nothing we can do, + // so just return immediately. + let old_file = wopen_cloexec(path, OFlag::O_RDONLY | OFlag::O_CREAT, LOCKED_FILE_MODE)?; + let opened_file_id = file_id_for_file(&old_file); + if initial_file_id != INVALID_FILE_ID && initial_file_id != opened_file_id { + // File ID changed (and not just because the file was created by us). + continue; + } + let Ok(potential_update) = rewrite(&old_file, &mut tmp_file) else { + // Retry if rewrite function failed. Because we do not hold a lock, this might be + // caused by concurrent modifications. + continue; + }; + + if potential_update.do_save { + update_metadata(&old_file, &tmp_file); + } + + let mut final_file_id = file_id_for_path(path); + if opened_file_id != final_file_id { + continue; + } + + // If we reach this point, the file ID did not change while we read the old file and wrote + // to `tmp_file`. Now we replace the old file with the `tmp_file`. + // Note that we cannot prevent races here. + // If the file is modified by someone else between the syscall for determining the [`FileId`] + // and the rename syscall, these modifications will be lost. + + if potential_update.do_save { + // Do not retry on rename failures, as it is unlikely that these will disappear if we retry. + rename(&tmp_name, path)?; + final_file_id = file_id_for_path(path); + } + // Note that this might not match the version of the file we just wrote. + // (If we did write.) + return Ok((final_file_id, potential_update)); + } + Err(std::io::Error::new(std::io::ErrorKind::Other, "Failed to update the file. Locking is disabled, and the fallback code did not succeed within the permissible number of attempts.")) +} diff --git a/src/history.rs b/src/history.rs index 8db59cbfb..d180f0164 100644 --- a/src/history.rs +++ b/src/history.rs @@ -2,47 +2,41 @@ //! //! 1. All history files are append-only. Data, once written, is never modified. //! -//! 2. A history file may be re-written ("vacuumed"). This involves reading in the file and writing a -//! new one, while performing maintenance tasks: discarding items in an LRU fashion until we reach -//! the desired maximum count, removing duplicates, and sorting them by timestamp (eventually, not -//! implemented yet). The new file is atomically moved into place via rename(). +//! 2. A history file may be re-written ("vacuumed"). This involves reading in the file and writing +//! a new one, while performing maintenance tasks: discarding items in an LRU fashion until we +//! reach the desired maximum count, removing duplicates, and sorting them by timestamp +//! (eventually, not implemented yet). The new file is atomically moved into place via `rename()`. //! -//! 3. History files are mapped in via mmap(). Before the file is mapped, the file takes a fcntl read -//! lock. The purpose of this lock is to avoid seeing a transient state where partial data has been -//! written to the file. +//! 3. History files are mapped in via `mmap()`. This allows only storing one `usize` per item (its +//! offset), and lazily loading items on demand, which reduces memory consumption. //! -//! 4. History is appended to under a fcntl write lock. -//! -//! 5. The chaos_mode boolean can be set to true to do things like lower buffer sizes which can -//! trigger race conditions. This is useful for testing. -//! -//! Locking on remote filesystems may hang for an unacceptably long time. For that reason, fish -//! does not take locks on the file if it believes the history file is on a remote filesystem, -//! or if the mmap fails with ENODEV, or if the first lock attempt takes excessively long. -//! Eliding locks means that two concurrent shell sessions with a remote history file may, in -//! rare cases with multiple simultaneous shell sessions, lose a history item; this is -//! considered preferable to hanging the the shell waiting for a lock. +//! 4. Accesses to the history file need to be synchronized. This is achieved by functionality in +//! `src/fs.rs`. By default, `flock()` is used for locking. If that is unavailable, an imperfect +//! fallback solution attempts to detect races and retries if a race is detected. use crate::{ - common::cstr2wcstring, env::EnvVar, fs::create_temporary_file, wcstringutil::trim, - wutil::fileid::file_id_for_path_or_error, + common::cstr2wcstring, + env::EnvVar, + fs::{ + lock_and_load, rewrite_via_temporary_file, LockedFile, LockingMode, PotentialUpdate, + WriteMethod, LOCKED_FILE_MODE, + }, + wcstringutil::trim, }; use std::{ borrow::Cow, collections::{BTreeMap, HashMap, HashSet}, ffi::CString, fs::File, - io::{BufRead, Read, Seek, SeekFrom, Write}, + io::{BufRead, Read, Write}, mem::MaybeUninit, num::NonZeroUsize, ops::ControlFlow, - os::{fd::AsRawFd, unix::fs::MetadataExt}, sync::{Arc, Mutex, MutexGuard}, time::{Duration, SystemTime, UNIX_EPOCH}, }; use bitflags::bitflags; -use libc::{fchown, flock, EINTR, LOCK_EX, LOCK_SH, LOCK_UN}; use lru::LruCache; use nix::{fcntl::OFlag, sys::stat::Mode}; use rand::Rng; @@ -60,18 +54,13 @@ operation_context::{OperationContext, EXPANSION_LIMIT_BACKGROUND}, parse_constants::{ParseTreeFlags, StatementDecoration}, parse_util::{parse_util_detect_errors, parse_util_unescape_wildcards}, - path::{ - path_get_config, path_get_data, path_get_data_remoteness, path_is_valid, DirRemoteness, - }, + path::{path_get_config, path_get_data, path_is_valid}, threads::{assert_is_background_thread, iothread_perform}, util::{find_subslice, get_rng}, wchar::prelude::*, wcstringutil::subsequence_in_string, wildcard::{wildcard_match, ANY_STRING}, - wutil::{ - file_id_for_file, file_id_for_path, wgettext_fmt, wrealpath, wrename, wstat, wunlink, - FileId, INVALID_FILE_ID, - }, + wutil::{file_id_for_file, wgettext_fmt, wrealpath, wstat, wunlink, FileId, INVALID_FILE_ID}, }; mod file; @@ -135,14 +124,6 @@ pub enum SearchDirection { /// Default buffer size for flushing to the history file. const HISTORY_OUTPUT_BUFFER_SIZE: usize = 64 * 1024; -/// The file access mode we use for creating history files -const HISTORY_FILE_MODE: Mode = Mode::from_bits_truncate(0o600); - -/// How many times we retry to save -/// Saving may fail if the file is modified in between our opening -/// the file and taking the lock -const MAX_SAVE_TRIES: usize = 1024; - pub const VACUUM_FREQUENCY: usize = 25; /// Output the contents `buffer` to `file` and clear the `buffer`. @@ -215,29 +196,6 @@ fn add_item(&mut self, item: HistoryItem) { } } -/// Returns the path for the history file for the given `session_id` -/// if `session_id` is non-empty. -/// If it is empty, `Ok(None)` will be returned. -/// An error is returned if obtaining the data directory failed. -/// Because the `path_get_data` function does not return error information, -/// we cannot provide more detail about the reason for the failure here. -/// If `suffix` is provided, append that suffix to the path; this is used for temporary files. -fn history_filename(session_id: &wstr, suffix: &wstr) -> std::io::Result> { - if session_id.is_empty() { - return Ok(None); - } - - let Some(mut result) = path_get_data() else { - return Err(std::io::Error::new(std::io::ErrorKind::NotFound, "Error obtaining data directory. This is a manually constructed error which does not indicate why this happened.")); - }; - - result.push('/'); - result.push_utfstr(session_id); - result.push_utfstr(L!("_history")); - result.push_utfstr(suffix); - Ok(Some(result)) -} - pub type PathList = Vec; pub type HistoryIdentifier = u64; @@ -406,11 +364,33 @@ struct HistoryImpl { old_item_offsets: Vec, } -/// If set, we gave up on file locking because it took too long. -/// Note this is shared among all history instances. -static ABANDONED_LOCKING: RelaxedAtomicBool = RelaxedAtomicBool::new(false); - impl HistoryImpl { + /// Returns the canonical path for the history file, or `Ok(None)` in private mode. + /// An error is returned if obtaining the data directory fails. + /// Because the `path_get_data` function does not return error information, + /// we cannot provide more detail about the reason for the failure here. + fn history_file_path(&self) -> std::io::Result> { + if self.name.is_empty() { + return Ok(None); + } + + let Some(mut path) = path_get_data() else { + return Err(std::io::Error::new(std::io::ErrorKind::NotFound, "Error obtaining data directory. This is a manually constructed error which does not indicate why this happened.")); + }; + + path.push('/'); + path.push_utfstr(&self.name); + path.push_utfstr(L!("_history")); + if let Some(canonicalized_path) = wrealpath(&path) { + Ok(Some(canonicalized_path)) + } else { + Err(std::io::Error::new( + std::io::ErrorKind::Other, + format!("wrealpath failed to produce a canonical version of '{path}'."), + )) + } + } + /// Add a new history item to the end. If `pending` is set, the item will not be returned by /// `item_at_index()` until a call to `resolve_pending()`. Pending items are tracked with an /// offset into the array of new items, so adding a non-pending item has the effect of resolving @@ -502,34 +482,22 @@ fn load_old_if_needed(&mut self) { self.loaded_old = true; let _profiler = TimeProfiler::new("load_old"); - if let Ok(Some(history_path)) = history_filename(&self.name, L!("")) { - let Ok(mut file) = wopen_cloexec(&history_path, OFlag::O_RDONLY, Mode::empty()) else { - return; - }; - - // Take a read lock to guard against someone else appending. This is released after - // getting the file's length. We will read the file after releasing the lock, but that's - // not a problem, because we never modify already written data. In short, the purpose of - // this lock is to ensure we don't see the file size change mid-update. - // - // We may fail to lock (e.g. on lockless NFS - see issue #685. In that case, we proceed - // as if it did not fail. The risk is that we may get an incomplete history item; this - // is unlikely because we only treat an item as valid if it has a terminating newline. - let locked = unsafe { Self::maybe_lock_file(&mut file, LOCK_SH) }; - self.file_contents = HistoryFileContents::create(&file).ok(); - self.history_file_id = if self.file_contents.is_some() { - file_id_for_file(&file) - } else { - INVALID_FILE_ID - }; - if locked { - unsafe { - Self::unlock_file(&mut file); + if let Ok(Some(history_path)) = self.history_file_path() { + match lock_and_load( + &history_path, + &LOCK_HISTORY_FILE, + HistoryFileContents::create, + ) { + Ok((file_id, file_contents)) => { + self.file_contents = Some(file_contents); + self.history_file_id = file_id; + let _profiler = TimeProfiler::new("populate_from_file_contents"); + self.populate_from_file_contents(); + } + Err(e) => { + FLOG!(history_file, "Error reading from history file:", e); } } - - let _profiler = TimeProfiler::new("populate_from_file_contents"); - self.populate_from_file_contents(); } } @@ -578,10 +546,9 @@ fn remove_ephemeral_items(&mut self) { } /// Given an existing history file, write a new history file to `dst`. - /// Returns false on error, true on success fn rewrite_to_temporary_file( &self, - existing_file: Option<&mut File>, + existing_file: &File, dst: &mut File, ) -> std::io::Result<()> { // We are reading FROM existing_file and writing TO dst @@ -591,32 +558,29 @@ fn rewrite_to_temporary_file( // Read in existing items (which may have changed out from underneath us, so don't trust our // old file contents). - if let Some(existing_file) = existing_file { - if let Ok(local_file) = HistoryFileContents::create(existing_file) { - let mut cursor = 0; - while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) { - // Try decoding an old item. - let Some(old_item) = local_file.decode_item(offset) else { - continue; - }; + if let Ok(local_file) = HistoryFileContents::create(existing_file) { + let mut cursor = 0; + while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) { + // Try decoding an old item. + let Some(old_item) = local_file.decode_item(offset) else { + continue; + }; - // If old item is newer than session always erase if in deleted. - if old_item.timestamp() > self.boundary_timestamp { - if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) { - continue; - } - lru.add_item(old_item); - } else { - // If old item is older and in deleted items don't erase if added by - // clear_session. - if old_item.is_empty() - || self.deleted_items.get(old_item.str()) == Some(&false) - { - continue; - } - // Add this old item. - lru.add_item(old_item); + // If old item is newer than session always erase if in deleted. + if old_item.timestamp() > self.boundary_timestamp { + if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) { + continue; } + lru.add_item(old_item); + } else { + // If old item is older and in deleted items don't erase if added by + // clear_session. + if old_item.is_empty() || self.deleted_items.get(old_item.str()) == Some(&false) + { + continue; + } + // Add this old item. + lru.add_item(old_item); } } } @@ -669,13 +633,9 @@ fn rewrite_to_temporary_file( /// Saves history by rewriting the file. fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { - // We want to rewrite the file, while holding the lock for as briefly as possible - // To do this, we speculatively write a file, and then lock and see if our original file changed - // Repeat until we succeed or give up - let Some(possibly_indirect_target_name) = history_filename(&self.name, L!(""))? else { + let Some(history_path) = self.history_file_path()? else { return Ok(()); }; - let tmp_name_template = history_filename(&self.name, L!(".XXXXXX"))?.unwrap(); FLOGF!( history, @@ -683,150 +643,43 @@ fn save_internal_via_rewrite(&mut self) -> std::io::Result<()> { self.new_items.len() - self.first_unwritten_new_item_index ); - // If the history file is a symlink, we want to rewrite the real file so long as we can find it. - let target_name = - wrealpath(&possibly_indirect_target_name).unwrap_or(possibly_indirect_target_name); + let rewrite = + |old_file: &File, tmp_file: &mut File| -> std::io::Result> { + self.rewrite_to_temporary_file(old_file, tmp_file)?; + Ok(PotentialUpdate { + do_save: true, + data: (), + }) + }; - // Make our temporary file - let (mut tmp_file, tmp_name) = create_temporary_file(&tmp_name_template)?; - let mut done = false; - for _i in 0..MAX_SAVE_TRIES { - if done { - break; - } + let (file_id, _) = rewrite_via_temporary_file(&history_path, &LOCK_HISTORY_FILE, rewrite)?; + self.history_file_id = file_id; - let target_file_before = wopen_cloexec( - &target_name, - OFlag::O_RDONLY | OFlag::O_CREAT, - HISTORY_FILE_MODE, - ); - if let Err(err) = target_file_before { - FLOG!(history_file, "Error opening history file:", err); - } + // We've saved everything, so we have no more unsaved items. + self.first_unwritten_new_item_index = self.new_items.len(); - let orig_file_id = target_file_before - .as_ref() - .map(file_id_for_file) - .unwrap_or(INVALID_FILE_ID); + // We deleted our deleted items. + self.deleted_items.clear(); - // Open any target file, but do not lock it right away - if let Err(err) = - self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) - { - // Failed to write, no good - FLOG!(history_file, "Error writing to temporary file:", err); - break; - } + // Our history has been written to the file, so clear our state so we can re-reference the + // file. + self.clear_file_state(); - // The crux! We rewrote the history file; see if the history file changed while we - // were rewriting it. Make an effort to take the lock before checking, to avoid racing. - // If the open fails, then proceed; this may be because there is no current history - let mut new_file_id = INVALID_FILE_ID; - - let mut target_file_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty()); - if let Ok(target_file_after) = target_file_after.as_mut() { - // critical to take the lock before checking file IDs, - // and hold it until after we are done replacing. - // Also critical to check the file at the path, NOT based on our fd. - // It's only OK to replace the file while holding the lock. - // Note any lock is released when target_file_after is closed. - unsafe { - Self::maybe_lock_file(target_file_after, LOCK_EX); - } - new_file_id = match file_id_for_path_or_error(&target_name) { - Ok(file_id) => file_id, - Err(err) => { - if err.kind() != std::io::ErrorKind::NotFound { - FLOG!(history_file, "Error re-opening history file:", err); - } - INVALID_FILE_ID - } - } - } - - let can_replace_file = new_file_id == orig_file_id || new_file_id == INVALID_FILE_ID; - if !can_replace_file { - // The file has changed, so we're going to re-read it - // Truncate our tmp_file so we can reuse it - if let Err(err) = tmp_file.set_len(0) { - FLOG!( - history_file, - "Error when truncating temporary history file:", - err - ); - } - if let Err(err) = tmp_file.seek(SeekFrom::Start(0)) { - FLOG!( - history_file, - "Error resetting cursor in temporary history file:", - err - ); - } - } else { - // The file is unchanged, or the new file doesn't exist or we can't read it - // We also attempted to take the lock, so we feel confident in replacing it - - // Ensure we maintain the ownership and permissions of the original (#2355). If the - // stat fails, we assume (hope) our default permissions are correct. This - // corresponds to e.g. someone running sudo -E as the very first command. If they - // did, it would be tricky to set the permissions correctly. (bash doesn't get this - // case right either). - if let Ok(target_file_after) = target_file_after.as_ref() { - if let Ok(md) = target_file_after.metadata() { - // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73 - if unsafe { fchown(tmp_file.as_raw_fd(), md.uid(), md.gid()) } == -1 { - FLOG!( - history_file, - "Error when changing ownership of history file:", - errno::errno() - ); - } - if let Err(e) = tmp_file.set_permissions(md.permissions()) { - FLOG!(history_file, "Error when changing mode of history file:", e); - } - } - } - - // Slide it into place - if wrename(&tmp_name, &target_name) == -1 { - FLOG!( - error, - wgettext_fmt!( - "Error when renaming history file: %s", - errno::errno().to_string() - ) - ); - } - - // We did it - done = true; - } - } - - // Ensure we never leave the old file around - let _ = wunlink(&tmp_name); - - if done { - // We've saved everything, so we have no more unsaved items. - self.first_unwritten_new_item_index = self.new_items.len(); - - // We deleted our deleted items. - self.deleted_items.clear(); - - // Our history has been written to the file, so clear our state so we can re-reference the - // file. - self.clear_file_state(); - } Ok(()) } /// Saves history by appending to the file. fn save_internal_via_appending(&mut self) -> std::io::Result<()> { - // Get the path to the real history file. - let Some(history_path) = history_filename(&self.name, L!(""))? else { + let Some(history_path) = self.history_file_path()? else { return Ok(()); }; - let history_path = wrealpath(&history_path).unwrap_or(history_path); + + 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, @@ -836,58 +689,20 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // No deleting allowed. assert!(self.deleted_items.is_empty()); - // If the file is different (someone vacuumed it) then we need to update our mmap. - let mut file_changed = false; + let mut locked_history_file = + LockedFile::new(LockingMode::Exclusive(WriteMethod::Append), &history_path)?; - // We are going to open the file, lock it, append to it, and then close it - // After locking it, we need to stat the file at the path; if there is a new file there, it - // means the file was replaced and we have to try again. - // Limit our max tries so we don't do this forever. - let mut num_attempts = 0; - let mut history_file = loop { - if num_attempts == MAX_SAVE_TRIES { - return Err(std::io::Error::new( - std::io::ErrorKind::Other, - "Number of unsuccessful attempts to open history file exceeds maximum.", - )); - } - // Return immediately if an error occurs here. - let mut file = wopen_cloexec( - &history_path, - OFlag::O_WRONLY | OFlag::O_APPEND, - Mode::empty(), - )?; + // Check if the file was modified since it was last read. + let file_id = file_id_for_file(locked_history_file.get()); + let file_changed = file_id != self.history_file_id; - // Exclusive lock on the entire file. This is released when we close the file (below). This - // may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that - // we may get interleaved history items, which is considered better than no history, or - // forcing everything through the slow copy-move mode. We try to minimize this possibility - // by writing with O_APPEND. - unsafe { - Self::maybe_lock_file(&mut file, LOCK_EX); - } - let file_id = file_id_for_file(&file); - if file_id_for_path(&history_path) == file_id { - // File IDs match, so the file we opened is still at that path - // We're going to use this fd - if file_id != self.history_file_id { - file_changed = true; - } - break file; - } - num_attempts += 1; - }; - - // We (hopefully successfully) took the exclusive lock. Append to the file. + // We took the exclusive lock. Append to the file. // Note that this is sketchy for a few reasons: // - Another shell may have appended its own items with a later timestamp, so our file may // no longer be sorted by timestamp. // - Another shell may have appended the same items, so our file may now contain // duplicates. // - // We cannot modify any previous parts of our file, because other instances may be reading - // those portions. We can only append. - // // Originally we always rewrote the file on saving, which avoided both of these problems. // However, appending allows us to save history after every command, which is nice! // @@ -911,7 +726,8 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // the file system. // Flushing and syncing each iteration adds overhead, // but hopefully there are not that many items to write when appending. - res = drain_buffer_into_file_and_flush(&mut buffer, &mut history_file); + res = drain_buffer_into_file_and_flush(&mut buffer, locked_history_file.get_mut()); + if res.is_err() { break; } @@ -923,11 +739,11 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> { // Since we just modified the file, update our history_file_id to match its current state // Otherwise we'll think the file has been changed by someone else the next time we go to // write. - // We don't update the mapping since we only appended to the file, and everything we + // We don't update `self.file_contents` since we only appended to the file, and everything we // appended remains in our new_items - self.history_file_id = file_id_for_file(&history_file); + self.history_file_id = file_id; - drop(history_file); + drop(locked_history_file); // If someone has replaced the file, forget our file state. if file_changed { @@ -946,6 +762,9 @@ fn save(&mut self, vacuum: bool) { return; } + // Compact our new items so we don't have duplicates. + self.compact_new_items(); + if self.name.is_empty() { // We're in the "incognito" mode. Pretend we've saved the history. self.first_unwritten_new_item_index = self.new_items.len(); @@ -953,16 +772,13 @@ fn save(&mut self, vacuum: bool) { self.clear_file_state(); } - // Compact our new items so we don't have duplicates. - self.compact_new_items(); - // Try saving. If we have items to delete, we have to rewrite the file. If we do not, we can // append to it. let mut ok = false; if !vacuum && self.deleted_items.is_empty() { // Try doing a fast append. if let Err(e) = self.save_internal_via_appending() { - FLOG!(history, "Appending to history failed", e); + FLOG!(history, "Appending to history failed:", e); } else { ok = true; } @@ -1048,7 +864,7 @@ fn is_empty(&mut self) -> bool { // If we have not loaded old items, don't actually load them (which may be expensive); just // stat the file and see if it exists and is nonempty. - let Ok(Some(where_)) = history_filename(&self.name, L!("")) else { + let Ok(Some(where_)) = self.history_file_path() else { return true; }; @@ -1104,7 +920,7 @@ fn clear(&mut self) { self.deleted_items.clear(); self.first_unwritten_new_item_index = 0; self.old_item_offsets.clear(); - if let Ok(Some(filename)) = history_filename(&self.name, L!("")) { + if let Ok(Some(filename)) = self.history_file_path() { wunlink(&filename); } self.clear_file_state(); @@ -1125,7 +941,7 @@ fn clear_session(&mut self) { /// file to the new history file. /// The new contents will automatically be re-mapped later. fn populate_from_config_path(&mut self) { - let Ok(Some(new_file)) = history_filename(&self.name, L!("")) else { + let Ok(Some(new_file)) = self.history_file_path() else { return; }; @@ -1148,7 +964,7 @@ fn populate_from_config_path(&mut self) { let mut dst_file = match wopen_cloexec( &new_file, OFlag::O_WRONLY | OFlag::O_CREAT, - HISTORY_FILE_MODE, + LOCKED_FILE_MODE, ) { Ok(file) => file, Err(err) => { @@ -1345,57 +1161,6 @@ fn size(&mut self) -> usize { let old_item_count = self.old_item_offsets.len(); return new_item_count + old_item_count; } - - /// Maybe lock a history file. - /// Returns `true` if successful, `false` if locking was skipped. - /// - /// # Safety - /// - /// `fd` and `lock_type` must be valid arguments to `flock(2)`. - unsafe fn maybe_lock_file(file: &mut File, lock_type: libc::c_int) -> bool { - assert!(lock_type & LOCK_UN == 0, "Do not use lock_file to unlock"); - - // Don't lock if it took too long before, if we are simulating a failing lock, or if our history - // is on a remote filesystem. - if ABANDONED_LOCKING.load() { - return false; - } - if path_get_data_remoteness() == DirRemoteness::remote { - return false; - } - - let (ok, start_time) = loop { - let start_time = SystemTime::now(); - if unsafe { flock(file.as_raw_fd(), lock_type) } != -1 { - break (true, start_time); - } - if errno::errno().0 != EINTR { - break (false, start_time); - } - }; - if let Ok(duration) = start_time.elapsed() { - if duration > Duration::from_millis(250) { - FLOG!( - warning, - wgettext_fmt!( - "Locking the history file took too long (%.3f seconds).", - duration.as_secs_f64() - ) - ); - ABANDONED_LOCKING.store(true); - } - } - ok - } - - /// Unlock a history file. - /// - /// # Safety - /// - /// `fd` must be a valid argument to `flock(2)` with `LOCK_UN`. - unsafe fn unlock_file(file: &mut File) { - libc::flock(file.as_raw_fd(), LOCK_UN); - } } fn string_could_be_path(potential_path: &wstr) -> bool { @@ -2081,3 +1846,5 @@ 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); diff --git a/src/history/file.rs b/src/history/file.rs index e7cd671dd..44efe9013 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -144,7 +144,11 @@ pub fn create(mut history_file: &File) -> std::io::Result { // filesystem does not support mapping. Treat this as a hint // that the filesystem is remote, and so disable locks for // the history file. - super::ABANDONED_LOCKING.store(true); + FLOG!( + history_file, + "Remote file system detected. Disabling history file locking.", + ); + super::LOCK_HISTORY_FILE.store(false); // Create an anonymous mapping and read() the file into it. map_anon(history_file, len)? } else { diff --git a/src/path.rs b/src/path.rs index 55f671a88..e41762add 100644 --- a/src/path.rs +++ b/src/path.rs @@ -667,7 +667,7 @@ fn create_dir_all_with_mode>(path: P, mode: u32) -> st } /// Return whether the given path is on a remote filesystem. -fn path_remoteness(path: &wstr) -> DirRemoteness { +pub fn path_remoteness(path: &wstr) -> DirRemoteness { let narrow = wcs2zstring(path); #[cfg(target_os = "linux")] { diff --git a/src/wutil/tests.rs b/src/wutil/tests.rs index 92d472c47..7ac0ab701 100644 --- a/src/wutil/tests.rs +++ b/src/wutil/tests.rs @@ -61,7 +61,7 @@ fn test_wdirname_wbasename() { #[serial] fn test_wwrite_to_fd() { let _cleanup = test_init(); - let (_fd, filename) = create_temporary_file(L!("/tmp/fish_test_wwrite.XXXXXX")).unwrap(); + let (_file, filename) = create_temporary_file(L!("/tmp/fish_test_wwrite.XXXXXX")).unwrap(); let filename = CString::new(filename.to_string()).unwrap(); let mut rng = get_rng(); let sizes = [1, 2, 3, 5, 13, 23, 64, 128, 255, 4096, 4096 * 2];