From 400e3a9feca335f0877c88a1ada8ff9dd79a2ba1 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 09:01:13 -0500 Subject: [PATCH] fix(format): resolve each hard link once A hard link's target may go through other hard links, and each was resolved again every time a path went through it. With each link's target naming the previous link twice (g/s{i} -> /g/s{i-1}/s{i-1}) the work doubled per link: finish() took 46 s for 26 links in a debug build, and 60 would never finish. Resolved links are now remembered, so the work is linear in the links, and a hard link met again while it is being resolved is reported as a cycle by name. The depth limit (64) still bounds the recursion through links not yet resolved. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/writer_tree.rs | 62 ++++++++++++++++--- .../clawhdf5/tests/writer_groups_interop.rs | 49 +++++++++++++++ 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/crates/clawhdf5-format/src/writer_tree.rs b/crates/clawhdf5-format/src/writer_tree.rs index 724ba70..275b73d 100644 --- a/crates/clawhdf5-format/src/writer_tree.rs +++ b/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 -/// against hard links whose targets name each other. +/// Depth of the chain of unresolved hard links followed while resolving one +/// 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 { + /// + /// 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], + from: usize, + target: &str, + depth: usize, + ) -> Result { 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> = b + .groups + .iter() + .map(|g| vec![Resolution::Todo; g.links.len()]) + .collect(); let mut resolved: Vec>> = 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 { diff --git a/crates/clawhdf5/tests/writer_groups_interop.rs b/crates/clawhdf5/tests/writer_groups_interop.rs index 27379bb..e8488c9 100644 --- a/crates/clawhdf5/tests/writer_groups_interop.rs +++ b/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 = (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]); + } +}