crates/clawhdf5-format/src/writer_tree.rs

@@ -17,8 +17,8 @@ use std::collections::BTreeMap;
 use crate::error::FormatError;
 use crate::type_builders::{AttrValue, DatasetBuilder, GroupBuilder, GroupItem};
 
-/// Hard links followed while resolving one hard-link target path. Guards
+/// Depth of the chain of unresolved hard links followed while resolving one
-/// against hard links whose targets name each other.
+/// hard-link target path (a bound on recursion; cycles are found exactly).
 const MAX_LINK_DEPTH: usize = 64;
 
 fn err(msg: String) -> FormatError {
@@ -256,10 +256,22 @@ impl Builder {
     }
 
     /// The object a hard link's `target` path names, from group `from`.
-    fn resolve(&self, from: usize, target: &str, depth: usize) -> Result<Obj, FormatError> {
+    ///
+    /// Hard links met on the way are resolved once and remembered in
+    /// `memo` (by group and link index), so a target that goes through
+    /// other hard links costs time linear in the links, not exponential; a
+    /// hard link met again while it is being resolved is a cycle.
+    fn resolve(
+        &self,
+        memo: &mut [Vec<Resolution>],
+        from: usize,
+        target: &str,
+        depth: usize,
+    ) -> Result<Obj, FormatError> {
         if depth > MAX_LINK_DEPTH {
             return Err(err(format!(
-                "hard link target {target:?}: too many hard links to follow (a cycle?)"
+                "hard link target {target:?}: more than {MAX_LINK_DEPTH} hard links \
+                 to follow"
             )));
         }
         let (mut cur, rest) = match target.strip_prefix('/') {
@@ -291,7 +303,22 @@ impl Builder {
             obj = match &grp.links[li].1 {
                 Target::Group(child) => Obj::Group(*child),
                 Target::Dataset(d) => Obj::Dataset(*d),
-                Target::Hard(p) => self.resolve(cur, p, depth + 1)?,
+                Target::Hard(p) => match memo[cur][li] {
+                    Resolution::Done(o) => o,
+                    Resolution::InProgress => {
+                        return Err(err(format!(
+                            "hard link target {target:?}: the hard link {:?} leads \
+                             back to itself (a cycle)",
+                            join(&grp.path, c)
+                        )));
+                    }
+                    Resolution::Todo => {
+                        memo[cur][li] = Resolution::InProgress;
+                        let o = self.resolve(memo, cur, p, depth + 1)?;
+                        memo[cur][li] = Resolution::Done(o);
+                        o
+                    }
+                },
                 Target::Soft(_) | Target::External { .. } => {
                     return Err(err(format!(
                         "hard link target {target:?} goes through a soft or external \
@@ -305,6 +332,14 @@ impl Builder {
     }
 }
 
+/// Where resolving one hard link has got to.
+#[derive(Clone, Copy)]
+enum Resolution {
+    Todo,
+    InProgress,
+    Done(Obj),
+}
+
 #[derive(Clone, Copy)]
 enum Obj {
     Group(usize),
@@ -325,14 +360,27 @@ pub(crate) fn build(root: GroupBuilder, default_track_order: bool) -> Result<Tre
     let mut group_refs = vec![0u32; b.groups.len()];
     let mut ds_refs = vec![0u32; b.datasets.len()];
     group_refs[0] = 1; // the superblock's reference to the root
+    let mut memo: Vec<Vec<Resolution>> = b
+        .groups
+        .iter()
+        .map(|g| vec![Resolution::Todo; g.links.len()])
+        .collect();
     let mut resolved: Vec<Vec<Option<Obj>>> = Vec::with_capacity(b.groups.len());
     for (gi, g) in b.groups.iter().enumerate() {
         let mut row = Vec::with_capacity(g.links.len());
-        for (_, t) in &g.links {
+        for (li, (_, t)) in g.links.iter().enumerate() {
             let obj = match t {
                 Target::Group(i) => Some(Obj::Group(*i)),
                 Target::Dataset(d) => Some(Obj::Dataset(*d)),
-                Target::Hard(p) => Some(b.resolve(gi, p, 0)?),
+                Target::Hard(p) => Some(match memo[gi][li] {
+                    Resolution::Done(o) => o,
+                    _ => {
+                        memo[gi][li] = Resolution::InProgress;
+                        let o = b.resolve(&mut memo, gi, p, 0)?;
+                        memo[gi][li] = Resolution::Done(o);
+                        o
+                    }
+                }),
                 Target::Soft(_) | Target::External { .. } => None,
             };
             match obj {

crates/clawhdf5/tests/writer_groups_interop.rs

@@ -827,3 +827,52 @@ fn a_link_too_big_for_dense_storage_is_an_error() {
     assert_eq!(out, "[11, 65001]");
     h5dump_ok(&path);
 }
+
+#[test]
+fn chained_hard_links_resolve_in_linear_time() {
+    skip_if_no_python!();
+    // Each link's target goes through the previous link twice. Resolving
+    // them without remembering resolved links doubled the work per link:
+    // 26 links took 46 s in a debug build, so 60 would never finish.
+    fn chain(reverse: bool) -> FileBuilder {
+        let mut b = FileBuilder::new();
+        let mut g = b.create_group("g");
+        g.create_dataset("v").with_i32_data(&[5]);
+        b.add_group(g.finish());
+        let mut order: Vec<usize> = (0..60).collect();
+        if reverse {
+            order.reverse();
+        }
+        for i in order {
+            if i == 0 {
+                b.add_hard_link("g/s0", "/g");
+            } else {
+                b.add_hard_link(&format!("g/s{i}"), &format!("/g/s{}/s{}", i - 1, i - 1));
+            }
+        }
+        b
+    }
+    let (tx, rx) = std::sync::mpsc::channel();
+    std::thread::spawn(move || {
+        let bytes = [false, true].map(|r| chain(r).finish().unwrap());
+        tx.send(bytes).unwrap();
+    });
+    let [forward, reverse] = rx
+        .recv_timeout(std::time::Duration::from_secs(60))
+        .expect("resolving 60 chained hard links took over a minute");
+
+    let dir = tempfile::tempdir().unwrap();
+    for (name, bytes) in [("forward.h5", forward), ("reverse.h5", reverse)] {
+        let path = dir.path().join(name).display().to_string();
+        std::fs::write(&path, bytes).unwrap();
+        let out = h5py(
+            &path,
+            "with h5py.File(path, 'r') as f:\n\
+             \x20   print(json.dumps([h5py.h5o.get_info(f['g'].id).rc, len(f['g']),\n\
+             \x20       int(f['g/s59/s30/s0/v'][0]), f['g/s59'] == f['g']]))",
+        );
+        assert_eq!(out, "[61, 61, 5, true]", "{name}");
+        let f = File::open(&path).unwrap();
+        assert_eq!(f.dataset("g/s59/s0/v").unwrap().read_i32().unwrap(), [5]);
+    }
+}