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

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

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());

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).

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`).