Save: a network drive on macOS that refuses the full flush gets a plain fsync (fixes #1620)
This commit is contained in:
parent
63686e659e
commit
5e8581e42f
|
|
@ -4689,6 +4689,7 @@ version = "0.10.0"
|
|||
dependencies = [
|
||||
"flate2",
|
||||
"proptest",
|
||||
"rustix 1.1.5",
|
||||
"serde",
|
||||
"serde_json",
|
||||
"thiserror 2.0.21",
|
||||
|
|
|
|||
|
|
@ -794,7 +794,7 @@ pub(crate) fn write_new_file(path: &str, bytes: &[u8]) -> Result<()> {
|
|||
may_write(path)?;
|
||||
let err = |e: std::io::Error| EngineError::Other(format!("{path}: {e}"));
|
||||
let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(path).map_err(err)?;
|
||||
let written = f.write_all(bytes).and_then(|()| f.sync_all());
|
||||
let written = f.write_all(bytes).and_then(|()| vectorcraft_format::sync_file(&f));
|
||||
drop(f);
|
||||
written.map_err(|e| {
|
||||
// Best effort: the partly written file is all there is to clean up.
|
||||
|
|
|
|||
|
|
@ -15,6 +15,11 @@ serde_json = { workspace = true }
|
|||
thiserror = { workspace = true }
|
||||
flate2 = { workspace = true }
|
||||
|
||||
# A plain fsync where a volume refuses the full flush (`sync_file`, atomic.rs). Already in
|
||||
# Cargo.lock (tempfile), so this adds no new crate.
|
||||
[target.'cfg(target_vendor = "apple")'.dependencies]
|
||||
rustix = { version = "1.1", default-features = false, features = ["std", "fs"] }
|
||||
|
||||
[dev-dependencies]
|
||||
vectorcraft-testkit = { workspace = true }
|
||||
proptest = { workspace = true }
|
||||
|
|
|
|||
|
|
@ -1,6 +1,9 @@
|
|||
//! Safe saving: a file is written to a temporary file beside it, then renamed over it, so a failed
|
||||
//! or interrupted write never leaves a damaged file behind (the old one stays as it was).
|
||||
//! [`write_new_with`] creates a file the same way without ever replacing one.
|
||||
//!
|
||||
//! On macOS `File::sync_all` is a full flush (`F_FULLFSYNC`), which SMB shares refuse with
|
||||
//! `ENOTSUP` while a plain `fsync` works: [`sync_file`] falls back to the plain one there (#1620).
|
||||
|
||||
use std::fs::{File, OpenOptions};
|
||||
use std::io::{self, Write as _};
|
||||
|
|
@ -19,7 +22,7 @@ pub fn write_atomic_with(path: &Path, fill: impl FnOnce(&mut File) -> io::Result
|
|||
let target = resolve_link(path)?;
|
||||
let tmp = temp_path(&target)?;
|
||||
let mut f = OpenOptions::new().write(true).create_new(true).open(&tmp)?;
|
||||
let filled = fill(&mut f).and_then(|()| f.sync_all());
|
||||
let filled = fill(&mut f).and_then(|()| sync_file(&f));
|
||||
drop(f);
|
||||
filled.and_then(|()| keep_permissions(&target, &tmp)).and_then(|()| std::fs::rename(&tmp, &target)).inspect_err(|_| {
|
||||
// Best effort: the temporary file is all there is to clean up.
|
||||
|
|
@ -45,7 +48,7 @@ fn write_new_linking(
|
|||
) -> io::Result<()> {
|
||||
let tmp = temp_path(path)?;
|
||||
let mut f = OpenOptions::new().write(true).create_new(true).open(&tmp)?;
|
||||
let filled = fill(&mut f).and_then(|()| f.sync_all());
|
||||
let filled = fill(&mut f).and_then(|()| sync_file(&f));
|
||||
drop(f);
|
||||
let placed = filled.and_then(|()| match link(&tmp, path) {
|
||||
Err(e) if e.kind() != io::ErrorKind::AlreadyExists => copy_new(&tmp, path),
|
||||
|
|
@ -60,7 +63,7 @@ fn write_new_linking(
|
|||
/// removes it again.
|
||||
fn copy_new(from: &Path, path: &Path) -> io::Result<()> {
|
||||
let mut to = OpenOptions::new().write(true).create_new(true).open(path)?;
|
||||
let copied = File::open(from).and_then(|mut f| io::copy(&mut f, &mut to)).and_then(|_| to.sync_all());
|
||||
let copied = File::open(from).and_then(|mut f| io::copy(&mut f, &mut to)).and_then(|_| sync_file(&to));
|
||||
drop(to);
|
||||
if copied.is_err() {
|
||||
// Best effort: the part copied is all there is to clean up.
|
||||
|
|
@ -69,6 +72,63 @@ fn copy_new(from: &Path, path: &Path) -> io::Result<()> {
|
|||
copied
|
||||
}
|
||||
|
||||
/// Flush `file` to stable storage (`sync_all`). On Apple platforms that is a full flush
|
||||
/// (`F_FULLFSYNC`); where the volume refuses it with `ENOTSUP` (SMB shares, #1620) this falls back
|
||||
/// to a plain `fsync`. Any other error, and a failed `fsync`, is returned as it is.
|
||||
pub fn sync_file(file: &File) -> io::Result<()> {
|
||||
match full_sync(file) {
|
||||
Err(e) if full_flush_unsupported(&e) => plain_fsync(file),
|
||||
r => r,
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether `e` is a volume refusing a full flush. Never off Apple platforms: there `sync_all` is
|
||||
/// already a plain `fsync`. Checked by its code because std maps `ENOTSUP` to `Uncategorized`.
|
||||
fn full_flush_unsupported(e: &io::Error) -> bool {
|
||||
cfg!(target_vendor = "apple") && e.raw_os_error() == Some(ENOTSUP)
|
||||
}
|
||||
|
||||
#[cfg(target_vendor = "apple")]
|
||||
fn plain_fsync(file: &File) -> io::Result<()> {
|
||||
#[cfg(test)]
|
||||
{
|
||||
PLAIN_FSYNCS.with(|c| c.set(c.get() + 1));
|
||||
if let Some(errno) = PLAIN_FSYNC_FAILS.with(std::cell::Cell::get) {
|
||||
return Err(io::Error::from_raw_os_error(errno));
|
||||
}
|
||||
}
|
||||
rustix::fs::fsync(file).map_err(io::Error::from)
|
||||
}
|
||||
|
||||
#[cfg(not(target_vendor = "apple"))]
|
||||
fn plain_fsync(file: &File) -> io::Result<()> {
|
||||
file.sync_all()
|
||||
}
|
||||
|
||||
fn full_sync(file: &File) -> io::Result<()> {
|
||||
#[cfg(test)]
|
||||
if REFUSE_FULL_SYNC.with(std::cell::Cell::get) {
|
||||
return Err(io::Error::from_raw_os_error(ENOTSUP));
|
||||
}
|
||||
file.sync_all()
|
||||
}
|
||||
|
||||
/// What macOS answers when a volume can't make a full flush (SMB shares).
|
||||
#[cfg(target_vendor = "apple")]
|
||||
const ENOTSUP: i32 = 45;
|
||||
#[cfg(not(target_vendor = "apple"))]
|
||||
const ENOTSUP: i32 = 95;
|
||||
|
||||
#[cfg(test)]
|
||||
thread_local! {
|
||||
/// Tests: every full flush on this thread fails as on an SMB share mounted by macOS.
|
||||
static REFUSE_FULL_SYNC: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
|
||||
/// Tests: the plain `fsync` fallback fails too, with this OS error.
|
||||
static PLAIN_FSYNC_FAILS: std::cell::Cell<Option<i32>> = const { std::cell::Cell::new(None) };
|
||||
/// Tests: how many plain `fsync` fallbacks were made on this thread.
|
||||
static PLAIN_FSYNCS: std::cell::Cell<u32> = const { std::cell::Cell::new(0) };
|
||||
}
|
||||
|
||||
/// A symbolic link keeps pointing at its file: write the file it names.
|
||||
fn resolve_link(path: &Path) -> io::Result<PathBuf> {
|
||||
match std::fs::symlink_metadata(path) {
|
||||
|
|
@ -137,6 +197,36 @@ mod tests {
|
|||
let _ = std::fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
/// A save to an SMB share (#1620): macOS refuses the full flush there with `ENOTSUP`, so the
|
||||
/// file is flushed with a plain `fsync` and saved. When that fails too the save fails as before,
|
||||
/// the old file stays and no temporary file is left. Elsewhere `sync_all` is already a plain
|
||||
/// `fsync`: a refusal is an error like any other.
|
||||
#[test]
|
||||
fn a_volume_that_refuses_the_full_flush_still_saves() {
|
||||
let d = dir("no-full-sync");
|
||||
let path = d.join("doc.vectorcraft");
|
||||
std::fs::write(&path, b"original").unwrap();
|
||||
let apple = cfg!(target_vendor = "apple");
|
||||
REFUSE_FULL_SYNC.with(|c| c.set(true));
|
||||
let before = PLAIN_FSYNCS.with(std::cell::Cell::get);
|
||||
let saved = write_atomic(&path, b"new");
|
||||
let created = write_new_with(&d.join("new.otf"), |f| f.write_all(b"created"));
|
||||
let fsyncs = PLAIN_FSYNCS.with(std::cell::Cell::get) - before;
|
||||
PLAIN_FSYNC_FAILS.with(|c| c.set(Some(5)));
|
||||
let failed = write_atomic(&path, b"lost");
|
||||
PLAIN_FSYNC_FAILS.with(|c| c.set(None));
|
||||
REFUSE_FULL_SYNC.with(|c| c.set(false));
|
||||
assert_eq!(saved.is_ok(), apple, "{saved:?}");
|
||||
assert_eq!(created.is_ok(), apple, "{created:?}");
|
||||
assert_eq!(fsyncs, if apple { 2 } else { 0 }, "each refused full flush falls back to fsync");
|
||||
assert!(failed.is_err(), "a failed fsync still fails the save");
|
||||
assert_eq!(std::fs::read(&path).unwrap(), if apple { &b"new"[..] } else { b"original" });
|
||||
let mut left = leftovers(&d, "doc.vectorcraft");
|
||||
left.retain(|n| n != "new.otf");
|
||||
assert!(left.is_empty(), "no temporary file is left: {left:?}");
|
||||
let _ = std::fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn write_new_never_replaces_what_is_there() {
|
||||
let d = dir("new");
|
||||
|
|
|
|||
|
|
@ -45,7 +45,7 @@ use serde::{Deserialize, Serialize};
|
|||
use vectorcraft_doc::{Document, ImageBlob};
|
||||
|
||||
#[cfg(not(target_arch = "wasm32"))]
|
||||
pub use atomic::{write_atomic, write_atomic_with, write_new_with};
|
||||
pub use atomic::{sync_file, write_atomic, write_atomic_with, write_new_with};
|
||||
use stream::{Gz, Indent, KeyScan, Out};
|
||||
|
||||
/// v4: large data after the JSON (v1 to v3 files still load).
|
||||
|
|
|
|||
Loading…
Reference in New Issue