From ae6b9604533c66376c949078078f127e1fb34baa Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 27 Sep 2026 07:31:57 -0500 Subject: [PATCH] facade: open_swmr reads live only files with the SWMR-write flag A file opened with open_swmr whose superblock does not have the SWMR-write flag (its writer has closed it) is now read exactly as File::open reads it: bounded by its recorded end of file, through the chunk cache, each operation tried once, and is_swmr_read() is false. Before, every file opened with open_swmr ignored its recorded end of file, so a closed file whose end of file is below its length (h5clear_fsm_persist_less.h5, or the mid-write fixture with its flags cleared) listed and read objects that File::open and libhdf5's plain reader refuse. Opening such a file is not retried once the superblock has been read. libhdf5's SWMR reader is looser than either (it reads the fixture's chunk index past the end of file even with the flags cleared); a test pins what h5py does and docs/design/swmr.md says why we follow the plain reader. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 ++ crates/clawhdf5/src/reader.rs | 60 ++++++++++++++---- crates/clawhdf5/tests/swmr_interop.rs | 88 +++++++++++++++++++++++++++ docs/design/swmr.md | 12 ++++ 4 files changed, 153 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d3275a..24527a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ Design: `docs/design/swmr.md`. would read as fill where the writer has since written). A file open for writing without SWMR is refused with `Error::Locked`, as libhdf5 refuses it. + Only a file whose superblock has the SWMR-write flag when it is opened is + read this way; any other file (its writer has closed it) reads exactly as + `File::open` reads it, bounded by its recorded end of file, and + `is_swmr_read()` is `false`. - **`Dataset::refresh()`** reads the dataset's object header again (`H5Drefresh`, h5py `Dataset.refresh()`), so `shape()` and later reads see the writer's appends; a handle keeps its extent until refreshed. diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 47788e7..4359300 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -142,7 +142,7 @@ impl FileData { /// bytes past the recorded end of file are not read, as in libhdf5. fn new(mut backing: Backing) -> Result<(Self, Superblock), Error> { if let Backing::Storage(storage) = backing { - return Self::new_storage(storage, false); + return Self::new_storage(storage, false, &mut None); } let whole = backing.whole_file().unwrap_or_default(); let (user_block, hdf5) = signature::split_user_block(whole)?; @@ -184,8 +184,19 @@ impl FileData { } /// [`Self::new`] for a [`Storage`] backend: the same checks, through - /// reads of the storage. A `live` file is never read as one slice. - fn new_storage(storage: SharedStorage, live: bool) -> Result<(Self, Superblock), Error> { + /// reads of the storage. + /// + /// With `swmr` ([`File::open_storage_swmr`]) the file is read live when + /// its superblock has the SWMR-write flag (version 3), as libhdf5's SWMR + /// reader reads it: reads end where the file ends at the time of each + /// read, and nothing is read as one slice. Any other file is read as + /// without `swmr`, bounded by its recorded end of file. Once the + /// superblock has been read, `flagged` says which it was. + fn new_storage( + storage: SharedStorage, + swmr: bool, + flagged: &mut Option, + ) -> Result<(Self, Superblock), Error> { let file_len = storage.len(); let base = signature::find_signature_in(&*storage)?; let mut data = Self { @@ -196,12 +207,16 @@ impl FileData { overlay: Vec::new(), image_error: None, contiguous: None, - live, + // Until the superblock says otherwise: its own reads are not + // bounded by an end of file it has not read yet. + live: swmr, }; // Worked out again below, once the end of file and any cache image // are known. data.contiguous = data.find_contiguous(); let superblock = Superblock::parse_in(&data, 0)?; + data.live = swmr && superblock.version >= 3 && superblock.is_swmr_write(); + *flagged = Some(data.live); data.end = base + superblock.data_end(base, file_len)?; match superblock_ext::cache_image_state_in(&data, &superblock)? { CacheImageState::Absent => {} @@ -515,10 +530,20 @@ impl File { /// attempt in which every structure read verified, so a torn read is /// an error (after the last attempt), never data. /// + /// All of this applies only to a file whose superblock (version 3) has + /// the SWMR-write flag set when it is opened: a file a SWMR writer has + /// open, or had and did not close. libhdf5's SWMR writer does not keep + /// the superblock's recorded end of file current, so for such a file it + /// is ignored, as libhdf5's SWMR reader ignores it. Any other file (one + /// whose writer has closed it, or that was never written in SWMR mode) + /// is read exactly as [`File::open`] reads it — bounded by its recorded + /// end of file, through the chunk cache, each operation tried once — + /// and [`is_swmr_read`](Self::is_swmr_read) is `false`. Which of the two + /// a handle is does not change after it is opened. + /// /// A file whose superblock says it is open for writing without SWMR is /// refused with [`Error::Locked`], as libhdf5 refuses it: such a writer - /// does not order its writes for readers. Any other file opens (one - /// whose writer has closed it reads like [`File::open`]). + /// does not order its writes for readers. pub fn open_swmr>(path: P) -> Result { let storage = crate::swmr::FileStorage::open(path.as_ref()).map_err(Error::Io)?; let mut f = Self::open_storage_swmr(Arc::new(storage))?; @@ -532,7 +557,16 @@ impl File { /// `HttpStorage`), does not show the writer's appends. pub fn open_storage_swmr(storage: SharedStorage) -> Result { let swmr = crate::swmr::Retries::default(); - let (data, superblock) = swmr.retry(|| FileData::new_storage(storage.clone(), true))?; + // Retried only while the superblock cannot be read, or says a SWMR + // writer has the file: an error opening any other file is final. + let (data, superblock) = swmr.retry(|| { + let mut flagged = None; + match FileData::new_storage(storage.clone(), true, &mut flagged) { + Ok(opened) => Ok(Ok(opened)), + Err(e) if flagged == Some(false) => Ok(Err(e)), + Err(e) => Err(e), + } + })??; if superblock.version >= 3 && superblock.is_write_access() && !superblock.is_swmr_write() { return Err(Error::Locked( "the file is open for writing without SWMR (libhdf5: \"file is already open \ @@ -550,15 +584,19 @@ impl File { }) } - /// Whether the file was opened with [`File::open_swmr`] or - /// [`File::open_storage_swmr`]. + /// Whether the file is read live: opened with [`File::open_swmr`] or + /// [`File::open_storage_swmr`] while its superblock had the SWMR-write + /// flag set. `false` for every other file, including one opened with + /// `open_swmr` whose writer had already closed it (it reads as + /// [`File::open`] reads it). pub fn is_swmr_read(&self) -> bool { self.data.live } /// How many times an operation on a SWMR-read file is tried (see - /// [`File::open_swmr`]); 100 unless set. Files not opened for SWMR - /// reading try every operation once. + /// [`File::open_swmr`]); 100 unless set. Files not read live + /// ([`is_swmr_read`](Self::is_swmr_read) `false`) try every operation + /// once. pub fn swmr_read_attempts(&self) -> u32 { self.swmr.attempts } diff --git a/crates/clawhdf5/tests/swmr_interop.rs b/crates/clawhdf5/tests/swmr_interop.rs index 58b1fac..c4f2e0b 100644 --- a/crates/clawhdf5/tests/swmr_interop.rs +++ b/crates/clawhdf5/tests/swmr_interop.rs @@ -166,6 +166,94 @@ fn open_swmr_refuses_a_file_open_for_writing_without_swmr() { assert!(!f.swmr_writer_active().unwrap()); } +/// The outcome of reading every dataset of `f` in full, as text: the +/// values, or the error. +fn read_all(f: &File) -> Vec { + ["a", "b"] + .iter() + .map(|n| match f.dataset(n).and_then(|d| d.read_f64()) { + Ok(v) => format!("{n}: {} values", v.len()), + Err(e) => format!("{n}: error {e}"), + }) + .collect() +} + +#[test] +fn open_swmr_reads_a_file_without_the_swmr_write_flag_as_file_open_does() { + // The mid-write copy with its flags cleared: a closed file that + // records an end of file of 715 in 6 030 bytes, its chunk indexes past + // that end. File::open (and libhdf5's plain reader) refuse what lies + // past the recorded end; open_swmr must too, and must not retry (the + // file is not live). See the h5py test below for libhdf5's SWMR reader. + let bytes = mid_write_copy_with_flags(0); + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let path = dir.path().join("closed_short_eof.h5"); + std::fs::write(&path, &bytes).unwrap(); + + let plain = File::open(&path).unwrap(); + let swmr = File::open_swmr(&path).unwrap(); + let swmr_storage = File::open_storage_swmr(Arc::new(bytes)).unwrap(); + let want = read_all(&plain); + assert!( + want.iter().all(|r| r.contains("error")), + "File::open reads past the recorded end: {want:?}" + ); + for f in [&swmr, &swmr_storage] { + assert!(!f.is_swmr_read()); + assert!(!f.swmr_writer_active().unwrap()); + let started = std::time::Instant::now(); + assert_eq!(read_all(f), want); + assert!(started.elapsed() < std::time::Duration::from_millis(100)); + assert_eq!(f.swmr_retries(), 0); + } + + // The same bytes with the SWMR-write flag (and write access) set are + // read live, past the recorded end. + let live = File::open_storage_swmr(Arc::new(mid_write_copy_with_flags(0x05))).unwrap(); + assert!(live.is_swmr_read()); + check_mid_write_copy(&live); +} + +#[test] +fn how_h5py_reads_the_closed_copy_past_its_recorded_end_of_file() { + skip_if_no_python!(); + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let path = dir.path().join("closed_short_eof.h5"); + std::fs::write(&path, mid_write_copy_with_flags(0)).unwrap(); + // libhdf5's plain reader refuses `a` (its chunk index is past the + // recorded end of file), as File::open and open_swmr do above. + // libhdf5's SWMR reader skips its end-of-allocation check for every + // file it opens (H5FD_read), and reads it; it still refuses an object + // *header* past the end ("address of object past end of allocation", + // H5O_protect). open_swmr does not copy that half-way rule for files + // without the SWMR-write flag: they read as File::open reads them. + let script = format!( + r#" +import h5py +for swmr in (False, True): + try: + with h5py.File("{p}", "r", swmr=swmr, locking=False) as f: + f["a"][()] + except Exception as e: + print("refused", swmr) + else: + print("read", swmr) +"#, + p = path.display() + ); + let out = Command::new(python()) + .args(["-c", &script]) + .output() + .unwrap(); + let stdout = String::from_utf8_lossy(&out.stdout); + assert_eq!( + stdout.split_whitespace().collect::>(), + ["refused", "False", "read", "True"], + "{}", + String::from_utf8_lossy(&out.stderr) + ); +} + /// A storage whose next `torn` reads come back garbled, as a read racing a /// rewrite of the structure can see them: the middle byte changed, or with /// `invert` every byte (signatures included). diff --git a/docs/design/swmr.md b/docs/design/swmr.md index c69fd86..959bfc4 100644 --- a/docs/design/swmr.md +++ b/docs/design/swmr.md @@ -76,6 +76,18 @@ writer keeps appending. What the format and the library guarantee: chunks it needs again. A cached index would hide new chunks, and a cached partial edge chunk would read as fill where the writer has since written data (unfiltered edge chunks are rewritten in place). + - Only a file whose superblock has the SWMR-write flag when it is opened + is read live. Any other file is read exactly as `File::open` reads it + (bounded by its recorded EOF, through the chunk cache, no retries), and + `is_swmr_read()` is `false`. libhdf5's SWMR reader is looser: it skips + the end-of-allocation check in `H5FD_read` for every file it opens, + flagged or not, yet still refuses an object header past the EOF + (`H5O_protect`, "address of object past end of allocation"). On a + closed file whose EOF is below its length (checked 2026-09-27, h5py + 3.16 / HDF5 2.0, the mid-write fixture with its flags cleared) it + therefore reads a dataset whose chunk index lies past the EOF, which + its plain reader refuses. We follow the plain reader there: the EOF of + a file no SWMR writer has open is what the file says it is. - A storage that caches blocks (`clawhdf5-remote`'s `BlockCache`) would serve stale bytes; `HttpStorage` also pins a file by ETag and length and refuses a changed file. Remote SWMR is out of scope.