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.

Sign up for GitHub

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

Jump to bottom

core/commands: Catch ErrResolveRecursion in non-recursive resolution #1351

Closed
wants to merge 1 commit into from
Closed

core/commands: Catch ErrResolveRecursion in non-recursive resolution #1351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/commands/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ The resolver will give:
depth = namesys.DefaultDepthLimit
}
output, err := resolver.ResolveN(req.Context().Context, name, depth)
if err != nil {
if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens in this case, then, if it skipps the error return? what's the UX?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Jun 09, 2015 at 04:05:21PM -0700, Juan Batiz-Benet wrote:

  •   if err != nil {

  •   if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {

what happens in this case, then, if it skipps the error return?
what's the UX?

It's not an error in the depth=1 case, it just means we didn't bottom
out on the resolution in a single step. Before this commit:

$ ipfs resolve /ipns/tremily.us
Error: could not resolve name (recursion limit exceeded).

After this commit:

$ ipfs resolve /ipns/tremily.us
/ipns/QmbqDJaoZYoGNw4dz7FqFDCf6q9EcHoMBQtoGViVFw2qv7

In both cases:

$ ipfs resolve --recursive /ipns/tremily.us
/ipfs/QmV2FrBtvue5ve7vxbAzKz3mTdWq8wfMNPwYd8d9KHksCF

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok. so it returns a value AND an error. can we maybe make sure the value is there then? bad things will happen if there is an error returned and a nil value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Jun 09, 2015 at 04:29:15PM -0700, Juan Batiz-Benet wrote:

@@ -63,7 +63,7 @@ The resolver will give:
depth = namesys.DefaultDepthLimit
}
output, err := resolver.ResolveN(req.Context().Context, name, depth)

  •   if err != nil {

  •   if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {

ah ok. so it returns a value AND an error. can we maybe make sure
the value is there then? bad things will happen if there is an error
returned and a nil value.

Only if the returned error is ErrResolveRecursion. I guess we could
add a check for that here 1 so we know we were ok by the time we got
to 2. But I'd rather avoid even more complicated error logic in the
client code.

Alternatively, we could push the depth=1 special casing down into the
resolve() helper, but I think the current symmetry makes more sense
there and I'd rather keep the “non-recursive resolution doesn't get an
/ipfs/ path” logic up in the command implementation.

Either way, we're going to have some funkyness somewhere, so you can
also just push me in the direction you think is most reasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why is that an error? If depth == 1 and that's the expected return, then it should not be an error, it should be the proper return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Jun 10, 2015 at 01:26:04AM -0700, Juan Batiz-Benet wrote:

  •   if err != nil {

  •   if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {

Wait, why is that an error? If depth == 1 and that's the expected
return, then it should not be an error, it should be the proper
return.

You said “Resolver, I'd like you to completely resolve this path to
/ipfs/…, and I'll let you take one step”. And the resolver says “I
took all the steps you let me take without trouble and got to ,
but I didn't make it to /ipfs/…”. It's then up to the caller to
decide whether it counts as success (“we successfully took all the
requested steps! :)”) or failure (“we didn't make it to /ipfs/… :(”).
For recursive resolution, I expect the more important condition is
“did we make it to /ipfs/…?”. But for non-recursive resolution, I
expect the more important condition is “was the step successfully
resolved to anything (/ipfs/… or otherwise)?”

As I said earlier 1, I think the current strategy of returning both
the resolved path and the ErrResolveRecursion error (“we successfully
took all our allotted steps to get to , but it's not /ipfs/…”)
makes sense, but then callers need logic like this to decide if they
consider that an error. But as I said in 1, I'd feel almost as good
about pushing this depth=1 special casing down into the resolve()
helper (and clients that didn't like that choice could explicitly
check for /ipfs/… paths in non-error, depth=1 cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Jun 10, 2015 at 01:26:47AM -0700, Juan Batiz-Benet wrote:

  •   if err != nil {

  •   if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {

Is the goal to do something like io.EOF which is returned even
when there is valid data returned?

Yeah, I guess that's a similar idea. In any case, we're saying
“here's how far we got: ”, and then either “and it was an
unequivocal success (nil error)” or “we have some concerns about our
success: ” where the concerns could be:

  • A resolution step failed. You should probably just give up.
  • All the resolution steps succeeded, but we hit our allotted
    recursion limit before finding an /ipfs/… path. You can give up if
    you like, but if this is really important to you, you can try
    another round of resolution or increase your depth limit in the
    future, and you might eventually resolve this path to /ipfs/….

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's another way to look at it:

  • resolve X means "dereference X"
  • resolve --depth=3 X means "dereference X 3 times"
  • resolve --recursive X means "dereference X until you cannot dereference any more"

If the command is satisfied, that's not really an error. We may also have IPNS values that dont even point to /ipfs/.... I think it's up to the user to figure out what to do with the returned value. This is a simpler interface with less complexity (having to figure out when to check the error, or not, etc).

In any case, i think the "return value and an error value" will be a bit ... error ... prone. the io.EOF thing is the only example that comes up in my mind from the go stdlib.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Jun 10, 2015 at 12:31:33PM -0700, Juan Batiz-Benet wrote:

  •   if err != nil {

  •   if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {

there's another way to look at it:

  • resolve X means "dereference X"
  • resolve --depth=3 X means "dereference X 3 times"

I don't think this makes sense. What if you can't dereference anymore
after 2 steps? Is that an error?

  • resolve --recursive X means "dereference X until you cannot
    dereference any more"

This has an implicit limit on the maximum allowed depth [1,2]. Do you
expect it to have different error handling from ‘--depth 32’? For
example, if the result after 32 steps is still in a mutable namespace,
should --recursive raise an error? Should ‘--depth 32’ raise an
error?

If the command is satisfied, that's not really an error. We may also
have IPNS values that dont even point to /ipfs/.... I think it's
up to the user to figure out what to do with the returned
value. This is a simpler interface with less complexity (having to
figure out when to check the error, or not, etc).

Are you suggesting we replace our current ResolveN interface 3 with:

ResolveNAllowUnresolved(ctx context.Context, name string, depth int) (value path.Path, err error)
ResolveNExpectResolved(ctx context.Context, name string, depth int) (value path.Path, err error)

(and likely similar for the implicit-depth-limited Resolve())? With
the dissection being that hitting the depth limit returns no path and
ErrResolveRecursion for *ExpectResolved and the final path and no
error for *AllowUnresolved. Maybe that seems more approachable, but
it means the “I'd like to try drilling deeper” case 4 has to start
over at the beginning (since it doesn't have access to the path
reached by *ExpectResolved). Or if you were using AllowUnresolved,
you'd need an additional check in the caller to decide if the path you
got back was resolved or not.

Copy link
Contributor Author

@wking wking Jul 13, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

res.SetError(err, cmds.ErrNormal)
return
}
Expand Down
2 changes: 1 addition & 1 deletion core/commands/ipns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ Resolve the value of another name:

resolver := namesys.NewRoutingResolver(n.Routing)
output, err := resolver.ResolveN(n.Context(), name, depth)
if err != nil {
if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {
res.SetError(err, cmds.ErrNormal)
return
}
Expand Down
2 changes: 1 addition & 1 deletion core/commands/resolve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ Resolve the value of another name recursively:
}

output, err := n.Namesys.ResolveN(n.Context(), name, depth)
if err != nil {
if err != nil && (err != namesys.ErrResolveRecursion || depth > 1) {
res.SetError(err, cmds.ErrNormal)
return
}
Expand Down