Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HAMT Preloader does not return error even if blocks do not load #54

Closed
aschmahmann opened this issue Jul 6, 2023 · 1 comment · Fixed by #55
Closed

HAMT Preloader does not return error even if blocks do not load #54

aschmahmann opened this issue Jul 6, 2023 · 1 comment · Fixed by #55

Comments

@aschmahmann
Copy link
Contributor

aschmahmann commented Jul 6, 2023

The HAMT Preloader ADL is meant to load all the blocks in the HAMT without following any links in the map's values.

Unfortunately, the way it does the preloading is by calling

traverse := n.Length()
if traverse == -1 {
return n, fmt.Errorf("could not fully explore hamt during preload")
}

While this would be fine if in fact -1 was returned during an error loading the HAMT.

However, it does not because the contract for that function for this function is that -1 is only returned if the node is not a list or a map and we know it is. https://github.com/ipld/go-ipld-prime/blob/65bfa53512f2328d19273e471ce4fd6d964055a2/datamodel/node.go#L131-L133

This means that when we have trouble loading a block we just continue on as if nothing happened which does not fulfill the contract of the preloader function

child, err := n.loadChild(pbLink)
if err != nil {
continue
}


This seems most readily resolvable by just using a different code path than Length() for doing the enumeration. Two that come to mind are:

  1. Copying/reusing the Length code but where we return errors
  2. Going through a map iterator that doesn't load the map value nodes
@willscott
Copy link
Collaborator

This makes sense. either of the options you propose sound pretty plausible. I'll take a look at a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants