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

wking · 2015-06-09T22:46:53Z

For depth-1, getting ErrResolveRecursion means the resolution was
successful and we hit our depth limit. That's what we want for
non-recursive resolution, so don't die with:

Error: could not resolve name (recursion limit exceeded).

in this case.

Fixes part of #1350. The rest of the resolution issue reported there
is #1307.

For depth-1, getting ErrResolveRecursion means the resolution was successful and we hit our depth limit. That's what we want for non-recursive resolution, so don't die with: Error: could not resolve name (recursion limit exceeded). in this case. License: MIT Signed-off-by: W. Trevor King <[email protected]>

jbenet · 2015-06-09T23:05:20Z

core/commands/dns.go

@@ -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) {


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

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

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.

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.

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.

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

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/….

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.

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.

From an IRC discussion with @jbenet on 2015-06-18: 16:14 <jbenet> i think the dns --recursive can just: resolve domains until it can't any more. 16:14 <jbenet> if it ends in /ipns/... cool. if it ends in /ipfs/... even cooler. 16:15 <wking> and if it ends in /tiddlywinks? 16:15 <jbenet> that's cool too. 16:16 <jbenet> the "dns resolver" only promised to resolve dns 16:16 <wking> how about for other resolvers? 16:16 <wking> E.g. if /ipns/QmSomeHash resolves to /tiddlywinks? 16:16 <jbenet> each should resolve only what they can 16:17 <jbenet> thats what the top level multi-protocol resolver is there to do 16:17 <wking> ok, so we just pull out the /ipfs/ special-case, and have callers handle that (if they expect an /ipfs/ entry) 16:17 <wking> that sounds good to me 16:17 <jbenet> yep

jbenet · 2015-06-12T00:51:44Z

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

No, you're done early. depth is a bound.

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?

it just needs to be clear what it's doing. --recursive may express that it is subject to a <max> = 32. --recursive that doesn't finish resolving may return the last and an error that it didn't finish resolving, but this error shouldn't be the case for --resolve=1. --resolve=1 is explicit and takes responsibility.

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

No.

You can resolve, hit the max, get the last value reached + error and issue resolve again. (so the error is only relevant when resolving using the all-protocol resolver AND the resolver knows it could keep going, but it has a max telling it not to. (This is not for the ResolveN case. ResolveN knows exactly how much it should resolve return the last value found)

wking · 2015-06-12T16:13:51Z

On Thu, Jun 11, 2015 at 05:51:45PM -0700, Juan Batiz-Benet wrote:

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?

it just needs to be clear what it's doing. --recursive may express
that it is subject to a <max> = 32. --recursive that doesn't
finish resolving may return the last and an error that it didn't
finish resolving, but this error shouldn't be the case for
--resolve=1. --resolve=1 is explicit and takes responsibility.

I think you mean --depth instead of --resolve? In that case we have
--recursive returning an error (we're still in a mutable namespace
after 32 steps) and --depth 32 not returning an error (we successfully
took 32 steps). Now we need to figure out where to put the switch
distinguishing between those two cases (in the resolver interface or
in the client code).

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

No.

You can resolve, hit the max, get the last value reached + error and
issue resolve again. (so the error is only relevant when resolving
using the all-protocol resolver AND the resolver knows it could
keep going, but it has a max telling it not to. (This is not for the
ResolveN case. ResolveN knows exactly how much it should resolve
return the last value found)

I'm not confident that I've parsed this correctly, but I think what
you're saying is:

Keep the Resolve() interface the same as it is now, returning both
the final path and ErrResolveRecursion if we hit the depth limit
before finding an /ipfs/… path (and maybe the “finding an /ipfs/…
path” condition should be “leaving the namespaces known to the
resolver”?).
Have ResolveN tread Resolve()'s ErrResolveRecursion case as a
success, and nil any such errors we get from base.go's resolve()
helper.

In that case I'm not sure how you phrase “I want to resolve this path
to completion in single steps”. Maybe by restoring CanResolve?

for {
child, err := resolver.ResolveN(ctx, path, 1)
if err != nil { // just ErrResolveFailed
return "", err // resolution failed
}
… do something with child …
if !resolver.CanResolve(child) { // resolution completed?
return child, nil
} // otherwise ResolveN hit its depth limit. Drill deeper
path = child
}

Personally, I prefer the current ResolveN API:

for {
child, err := resolver.ResolveN(ctx, path, 1)
if err != nil && err != ErrResolveRecursion {
return "", err // resolution failed
}
… do something with child …
if err == nil { // resolution completed
return child, nil
} // otherwise we hit our depth limit. Drill deeper
path = child
}

since it doesn't need a separate endpoint and function call for
CanResolve.

whyrusleeping · 2015-10-18T07:35:52Z

closing, old PR cleanup time.

jbenet added the status/in-progress In progress label Jun 9, 2015

wking mentioned this pull request Jun 9, 2015

Strange command behaviors #1350

Closed

jbenet reviewed Jun 9, 2015
View reviewed changes

whyrusleeping mentioned this pull request Jun 23, 2015

Sprint M ipfs/team-mgmt#17

Closed

34 tasks

jbenet added the need/triage Needs initial labeling and prioritization label Jun 30, 2015

whyrusleeping mentioned this pull request Jul 1, 2015

Sprint N ipfs/team-mgmt#20

Closed

44 tasks

whyrusleeping closed this Oct 18, 2015

jbenet removed the status/in-progress In progress label Oct 18, 2015

Kubuxu deleted the tk/resolve-fixes branch February 27, 2017 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wking commented Jun 9, 2015

jbenet Jun 9, 2015

wking Jun 9, 2015

jbenet Jun 9, 2015

wking Jun 10, 2015

jbenet Jun 10, 2015

wking Jun 10, 2015

wking Jun 10, 2015

jbenet Jun 10, 2015

wking Jun 10, 2015

wking Jul 13, 2015 via email

jbenet commented Jun 12, 2015

wking commented Jun 12, 2015

whyrusleeping commented Oct 18, 2015

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

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

Conversation

wking commented Jun 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Jul 13, 2015 via email

Choose a reason for hiding this comment

jbenet commented Jun 12, 2015

wking commented Jun 12, 2015

whyrusleeping commented Oct 18, 2015