edit: plan every edit from the file the editor holds, not its path
FileEditor re-opened its path to plan each edit but wrote through the file it held open, and the Python 'r+' handle re-opened the path after every edit to read. When the path came to name another file between edits (a rename or replacement, or a relative path after os.chdir), an edit was laid out from the other file's metadata and written into the held one, corrupting it, and later reads came from the other file (the review's repro: h5py then reports "invalid dataset size, likely file corruption"). The editor now plans from a mapping of its own file (a clone of the held descriptor, dropped before the edit writes) and canonicalises its path at open. New FileEditor::reader() opens the held file anew for reading, without sharing the editor's flock (a mapping of a cloned descriptor holds the lock until unmapped): through /proc/self/fd on Linux, which follows a renamed file; elsewhere by path, refused on Unix when the path no longer names the held file. The Python handle reads through it and keeps no path; a 'w' file is written at the absolute path it was opened with. Tests: edit_tests.rs edits_go_to_the_file_held_not_the_path; test_edit.py test_relative_path_and_chdir and test_path_replaced_between_edits. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
@@ -110,7 +110,9 @@ impl PyFile {
|
||||
"w" => Ok(Self {
|
||||
filename,
|
||||
inner: Some(FileInner::Write(WriteState {
|
||||
path: PathBuf::from(path),
|
||||
// Absolute now: the file is written at close, possibly
|
||||
// after the working directory changed.
|
||||
path: std::path::absolute(path).unwrap_or_else(|_| PathBuf::from(path)),
|
||||
root_datasets: Vec::new(),
|
||||
root_attrs: Arc::new(Mutex::new(Vec::new())),
|
||||
groups: Vec::new(),
|
||||
|
||||
@@ -8,7 +8,8 @@
|
||||
//!
|
||||
//! A file opened with `'r+'` also holds a [`FileEditor`]. An edit takes the
|
||||
//! file's write lock, so no read runs while the file changes underneath it,
|
||||
//! and reopens the file afterwards: reads after an edit see the new bytes
|
||||
//! and reopens the file afterwards, through the editor's own open file
|
||||
//! rather than its path: reads after an edit see the new bytes
|
||||
//! (a grown file, a new dataspace), never a stale mapping or chunk cache.
|
||||
//! Objects that cache something an edit can change compare
|
||||
//! [`Handle::generation`] with the value they cached it at.
|
||||
@@ -16,7 +17,6 @@
|
||||
//! Lock discipline (no deadlock with the GIL): the file lock is only taken
|
||||
//! with the GIL released, and code that holds it never touches Python.
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::sync::atomic::{AtomicU64, Ordering};
|
||||
use std::sync::{Arc, Mutex, PoisonError, RwLock};
|
||||
|
||||
@@ -28,8 +28,9 @@ use crate::{panic_text, to_py_err};
|
||||
|
||||
/// Where the file's bytes come from.
|
||||
pub(crate) enum Source {
|
||||
/// A local path (memory-mapped).
|
||||
Local(PathBuf),
|
||||
/// A local file (memory-mapped). Its path is not kept: nothing reopens
|
||||
/// it by path (see `open_editable`).
|
||||
Local,
|
||||
/// A URL, read through `clawhdf5-remote`'s block cache.
|
||||
Remote {
|
||||
url: String,
|
||||
@@ -74,25 +75,23 @@ impl Handle {
|
||||
/// A local file, read-only.
|
||||
pub(crate) fn open_local(py: Python<'_>, path: &str) -> PyResult<Arc<Self>> {
|
||||
let file = py.detach(|| crate::no_panic(|| File::open(path).map_err(to_py_err)))?;
|
||||
Ok(Self::new(file, Source::Local(PathBuf::from(path)), None))
|
||||
Ok(Self::new(file, Source::Local, None))
|
||||
}
|
||||
|
||||
/// A local file, open for in-place editing (`'r+'`): the editor takes
|
||||
/// the file's exclusive lock and checks that it can edit the file, then
|
||||
/// the file is opened for reading.
|
||||
/// the file is read through the editor's own file — never by path
|
||||
/// again, so a later `os.chdir` or a rename or replacement of the path
|
||||
/// cannot make reads (or the editor's plans) come from another file.
|
||||
pub(crate) fn open_editable(py: Python<'_>, path: &str) -> PyResult<Arc<Self>> {
|
||||
let (file, editor) = py.detach(|| {
|
||||
crate::no_panic(|| {
|
||||
let editor = FileEditor::open(path).map_err(to_py_err)?;
|
||||
let file = File::open(path).map_err(to_py_err)?;
|
||||
let file = editor.reader().map_err(to_py_err)?;
|
||||
Ok((file, editor))
|
||||
})
|
||||
})?;
|
||||
Ok(Self::new(
|
||||
file,
|
||||
Source::Local(PathBuf::from(path)),
|
||||
Some(editor),
|
||||
))
|
||||
Ok(Self::new(file, Source::Local, Some(editor)))
|
||||
}
|
||||
|
||||
/// A remote file (`http(s)://`, `s3://`, ...).
|
||||
@@ -153,7 +152,7 @@ impl Handle {
|
||||
pub(crate) fn remote_storage(&self) -> Option<&clawhdf5_remote::RemoteStorage> {
|
||||
match &self.source {
|
||||
Source::Remote { storage, .. } => Some(storage),
|
||||
Source::Local(_) => None,
|
||||
Source::Local => None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -161,7 +160,7 @@ impl Handle {
|
||||
pub(crate) fn redacted_url(&self) -> Option<String> {
|
||||
match &self.source {
|
||||
Source::Remote { url, .. } => Some(clawhdf5_remote::redact_url(url)),
|
||||
Source::Local(_) => None,
|
||||
Source::Local => None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -185,14 +184,12 @@ impl Handle {
|
||||
let Some(editor) = &self.editor else {
|
||||
return Err(PyOSError::new_err(match self.source {
|
||||
Source::Remote { .. } => "remote files are read-only",
|
||||
Source::Local(_) => {
|
||||
"the file is open read-only; open it with mode 'r+' to change it"
|
||||
}
|
||||
Source::Local => "the file is open read-only; open it with mode 'r+' to change it",
|
||||
}));
|
||||
};
|
||||
let Source::Local(path) = &self.source else {
|
||||
if matches!(self.source, Source::Remote { .. }) {
|
||||
return Err(PyOSError::new_err("remote files are read-only"));
|
||||
};
|
||||
}
|
||||
py.detach(|| {
|
||||
let mut ed = editor.lock().unwrap_or_else(PoisonError::into_inner);
|
||||
let ed = ed
|
||||
@@ -202,7 +199,8 @@ impl Handle {
|
||||
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(ed)));
|
||||
// Drop the old mapping and its chunk cache before reopening.
|
||||
*file = None;
|
||||
let reopened = std::panic::catch_unwind(|| File::open(path));
|
||||
// Through the editor's file, not the path (see `open_editable`).
|
||||
let reopened = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| ed.reader()));
|
||||
self.generation.fetch_add(1, Ordering::AcqRel);
|
||||
match reopened {
|
||||
Ok(Ok(f)) => *file = Some(f),
|
||||
|
||||
Reference in New Issue
Block a user