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.