diff --git a/CHANGELOG.md b/CHANGELOG.md index 24527a8..30f7888 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,14 +36,16 @@ Design: `docs/design/swmr.md`. see the writer's appends; a handle keeps its extent until refreshed. - **Bounded retries.** On a SWMR-read file, an operation (open, lookups, refresh, reads) that fails with an error a concurrent write can cause — - any format error except a wrong name or selection, an unsupported - feature or a bad argument — is run again from the start, up to + the failures libhdf5's SWMR reader retries: a checksum mismatch, a read + past the file's current end, an object header prefix that does not + decode — is run again from the start, up to `File::swmr_read_attempts()` times (`SWMR_READ_ATTEMPTS` = 100, libhdf5's default metadata read attempts for SWMR readers, `H5Pset_metadata_read_attempts`; `set_swmr_read_attempts` changes it), pausing 1 µs doubling to 10 ms between attempts. Results come only from an attempt in which every structure verified, so a torn read is at worst - an error, never data. `File::swmr_retries()` counts the retries. + an error, never data. Any other error is returned at once. + `File::swmr_retries()` counts the retries. `File::swmr_writer_active()` reads the superblock flags again to tell when the writer has closed the file. - Tests (`crates/clawhdf5/tests/swmr_interop.rs`): the mid-write copy diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 4359300..b46b1b9 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -520,15 +520,15 @@ impl File { /// [`Dataset::refresh`] to see the writer's appends. /// /// An operation that fails with an error a concurrent write can cause - /// (a checksum mismatch, a short read, a bad signature or version byte, - /// a chunk or chunk index that does not decode: every format error but - /// a wrong path or selection, an unsupported feature or a bad - /// argument) is run again from the start, up to + /// — the failures libhdf5's SWMR reader retries: a checksum mismatch, a + /// read past the file's current end, an object header prefix (signature, + /// version) that does not decode — is run again from the start, up to /// [`swmr_read_attempts`](Self::swmr_read_attempts) times (100 by /// default, libhdf5's default for SWMR readers), with a pause of 1 µs /// doubling up to 10 ms between attempts. Data is only returned from an /// attempt in which every structure read verified, so a torn read is - /// an error (after the last attempt), never data. + /// an error (after the last attempt), never data. Every other error is + /// returned at once. /// /// 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 diff --git a/crates/clawhdf5/src/swmr.rs b/crates/clawhdf5/src/swmr.rs index 6d6ba76..bf0112d 100644 --- a/crates/clawhdf5/src/swmr.rs +++ b/crates/clawhdf5/src/swmr.rs @@ -103,10 +103,8 @@ impl Storage for FileStorage { } /// Whether `e` can be caused by reading a structure while a SWMR writer -/// rewrites or has not yet finished writing it — a checksum mismatch, a -/// short read, a bad signature, a chunk index or chunk that does not -/// decode, ... (see [`is_transient_format`]) — so that the operation is -/// worth running again. +/// rewrites it or has not yet written it, so that the operation is worth +/// running again (see [`is_transient_format`]). pub(crate) fn is_transient(e: &Error) -> bool { match e { Error::Format(f) => is_transient_format(f), @@ -115,28 +113,31 @@ pub(crate) fn is_transient(e: &Error) -> bool { } } -/// A read that raced a write can garble any field of a structure, and the -/// parsers report that as whatever check fails first (the checksum, a -/// signature, a version byte, a size), so every format error counts except -/// those that bytes read later cannot change: a name or selection the -/// caller got wrong, a feature this reader does not support, an argument -/// that does not fit. +/// The failures libhdf5's SWMR reader retries, and nothing else. libhdf5 +/// (`H5C__load_entry`) reads a metadata structure again when its checksum +/// fails, and when the prefix it decodes before the checksum to learn the +/// structure's size does not decode (for an object header, its signature +/// and version: a header whose every byte is garbled fails there). A read +/// past the file's current end is short here; libhdf5 reads zeros there, +/// which then fail the checksum. So: +/// +/// - [`FormatError::ChecksumMismatch`], of any checksummed structure; +/// - [`FormatError::UnexpectedEof`], a read past the current end; +/// - [`FormatError::InvalidObjectHeaderSignature`] and +/// [`FormatError::InvalidObjectHeaderVersion`], the object header prefix. +/// +/// Every other error — an unsupported version or message, a file that is not +/// HDF5, a structure that is corrupt behind a valid checksum — is returned +/// at once: a concurrent write does not cause it, and retrying it only +/// costs up to a second of pauses. fn is_transient_format(e: &FormatError) -> bool { use FormatError as F; - !matches!( + matches!( e, - F::PathNotFound(_) - | F::SelectionOutOfBounds(_) - | F::UnsupportedFilter(_) - | F::ExternalDataFilesUnsupported - | F::ExternalLinkUnsupported { .. } - | F::ContiguousStorageRequired(_) - | F::TypeMismatch { .. } - | F::DataSizeMismatch { .. } - | F::SerializationError(_) - | F::CompressionError(_) - | F::DuplicateDatasetName(_) - | F::InvalidLinkName + F::ChecksumMismatch { .. } + | F::UnexpectedEof { .. } + | F::InvalidObjectHeaderSignature + | F::InvalidObjectHeaderVersion(_) ) } @@ -238,6 +239,69 @@ mod tests { assert_eq!(&*s.read_at(3, 100).unwrap(), b"lo, world"); } + #[test] + fn permanent_errors_are_returned_at_once() { + // Errors a concurrent write does not cause: none is retried, so + // none costs the pauses (about 0.9 s for 100 attempts). + let permanent = [ + FormatError::SignatureNotFound, + FormatError::UnsupportedVersion(9), + FormatError::UnsupportedMessage(0x99), + FormatError::TruncatedFile { + stored_eof: 10, + actual_len: 5, + }, + FormatError::InvalidDatatypeClass(15), + FormatError::InvalidLayoutVersion(9), + FormatError::InvalidBTreeSignature, + FormatError::ChunkedReadError("x".into()), + FormatError::DecompressionError("x".into()), + FormatError::Fletcher32Mismatch { + expected: 1, + computed: 2, + }, + ]; + let n = AtomicU64::new(0); + let started = std::time::Instant::now(); + for e in permanent { + assert!(!is_transient_format(&e), "{e:?}"); + let mut calls = 0; + let r: Result<(), Error> = retry(SWMR_READ_ATTEMPTS, &n, || { + calls += 1; + Err(Error::Format(e.clone())) + }); + assert!(r.is_err()); + assert_eq!(calls, 1, "{e:?}"); + } + assert_eq!(n.load(Ordering::Relaxed), 0); + assert!(started.elapsed() < Duration::from_millis(50)); + assert!(!is_transient(&Error::Io(std::io::Error::other("x")))); + + // The transient ones are retried to the limit. + for e in [ + FormatError::ChecksumMismatch { + expected: 1, + computed: 2, + }, + FormatError::UnexpectedEof { + expected: 8, + available: 0, + }, + FormatError::InvalidObjectHeaderSignature, + FormatError::InvalidObjectHeaderVersion(0x4f), + ] { + let mut calls = 0; + let _: Result<(), Error> = retry(3, &n, || { + calls += 1; + Err(Error::Format(e.clone())) + }); + assert_eq!(calls, 3, "{e:?}"); + } + assert!(is_transient(&Error::Io( + std::io::ErrorKind::UnexpectedEof.into() + ))); + } + #[test] fn retry_runs_again_only_for_transient_errors() { let n = AtomicU64::new(0); @@ -287,7 +351,7 @@ mod tests { outer += 1; retry(3, &n, || { inner += 1; - Err(Error::Format(FormatError::SignatureNotFound)) + Err(Error::Format(FormatError::InvalidObjectHeaderVersion(0x76))) }) }); assert!(r.is_err()); diff --git a/crates/clawhdf5/tests/swmr_interop.rs b/crates/clawhdf5/tests/swmr_interop.rs index c4f2e0b..ca97931 100644 --- a/crates/clawhdf5/tests/swmr_interop.rs +++ b/crates/clawhdf5/tests/swmr_interop.rs @@ -254,6 +254,31 @@ for swmr in (False, True): ); } +#[test] +fn open_swmr_returns_a_permanent_error_at_once() { + // Not HDF5 at all: SignatureNotFound is not something a writer + // causes, so it is not retried (100 attempts would pause about 0.9 s). + let started = std::time::Instant::now(); + let err = File::open_storage_swmr(Arc::new(vec![7u8; 4096])).unwrap_err(); + assert!( + matches!( + err, + clawhdf5::Error::Format(clawhdf5_format::error::FormatError::SignatureNotFound) + ), + "{err}" + ); + assert!(started.elapsed() < std::time::Duration::from_millis(100)); + + // A live file: a lookup of a name it does not have fails at once, and + // a torn object header read (below) is retried. + let f = File::open_storage_swmr(Arc::new(mid_write_copy_with_flags(0x05))).unwrap(); + assert!(f.is_swmr_read()); + let started = std::time::Instant::now(); + assert!(f.dataset("no_such").is_err()); + assert!(started.elapsed() < std::time::Duration::from_millis(100)); + assert_eq!(f.swmr_retries(), 0); +} + /// 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 959bfc4..9f52876 100644 --- a/docs/design/swmr.md +++ b/docs/design/swmr.md @@ -103,14 +103,16 @@ writer keeps appending. What the format and the library guarantee: it), sleeping 1 µs, 2 µs, … up to 10 ms between attempts (under a second in all). libhdf5 retries the one structure whose checksum failed; we retry the whole operation, because the parsers are pure functions of the - bytes they read. Which errors: a garbled read can fail whichever check - the parser makes first — a checksum, a signature, a version byte, a - size (a test that garbles every byte of a header got - `InvalidObjectHeaderVersion`, not a checksum error) — so every format - error counts except those later bytes cannot change: a path or selection - the caller got wrong, an unsupported filter or external storage, a bad - argument. Those, and a live file's permanent errors of the other kinds - after the last attempt, are returned; non-live files never retry. + bytes they read. Which errors: those libhdf5 retries (`H5C__load_entry`) + — a checksum mismatch, and a failure to decode the prefix it reads + before the checksum to size the structure (for an object header its + signature and version: a test that garbles every byte of a header gets + `InvalidObjectHeaderVersion`) — and a read past the file's current end + (short here; libhdf5 reads zeros there, which fail the checksum). Every + other error (an unsupported version or message, a file that is not HDF5, + a structure corrupt behind a valid checksum) is returned at once: an + earlier version retried nearly every format error, so a permanent one + cost about 0.9 s of pauses. Non-live files never retry. Results are only returned from a run where every structure verified, so a torn metadata read is an error, never data. `File::swmr_retries()` counts the retries (libhdf5: `H5Fget_metadata_read_retry_info`).