core: add context.Context param to core.Resolve() #1189

cryptix · 2015-05-03T00:50:22Z

Following the discussion of #1150, we need to be able to pass a context to core.Resolve() so that a user is able to stop it.

cryptix · 2015-05-03T00:53:06Z

core/pathresolver.go

+	return r.resolveRecurse(0)
+}
+
+type resolver struct {


i just added this type to make the recursive call easier to grasp.

whyrusleeping · 2015-05-03T03:23:03Z

core/commands/publish.go

@@ -89,7 +91,8 @@ Publish an <ipfs-path> to another public key (not implemented):
 		}

 		// TODO n.Keychain.Get(name).PrivKey
-		output, err := publish(n, n.PrivateKey, p)
+		// TODO(cryptix): is req.Context().Context a child of n.Context()?


the top level context stuff around here is really confusing.. it uses this context group thing if i remember correctly

@jbenet can you tell us?

apparently req.Context().Context() is not yet a child of n.Context(): https://github.com/ipfs/go-ipfs/blob/master/commands/http/handler.go#L52-L54 -- it should be.

and yes it's very confusing :(

@jbenet ohai cmdlib 😞 .. but fixed 😄. Do I need to mirror this change on the offline mode, too?

@cryptix

Do I need to mirror this change on the offline mode, too?

probably? i can think about this more carefully tomorrow.

jbenet · 2015-05-03T18:37:49Z

path/resolver.go

+				TODO(cryptix): we wrapped a new context for each name iteration
+				is this correct?
+				moved it out of the loop for now
+			*/


i think this used to be right. we should probably give longer names more time. example: if resolution takes ~10s per hop, 1min is not going to cut it for paths larger than 6. (resolution should be much faster, sure, but remember that resolution might go across the routing system).

in the end, this probably should come from the context coming in, and not have a timeout internally. (but in that case we must make sure all source calls do have proper timeouts

That makes sense. Changed it back.

jbenet · 2015-05-03T20:18:48Z

This PR is fighting the good fight :) 👍

Some notes (not for this PR, but for when making the core/shell) api:

More of these functions should take their own contexts.
should be cancellation works when either (a) the request is cancelled, (b) the node itself is cancelled/stopped ("multiple parents" -- https://godoc.org/github.com/jbenet/go-context/ext#WithParents)
In many cases, the addition of the context "pollutes" the api, so can have parallel calls[1]

[1] since contexts are pervasive and we'd like to be able to have them in many function calls, we'd like to be able to add them all over to expose the ability to cancel things. But this does pollute the API all over the place, and make it harder to glean for the first time user. One way is to have a package that mirrors the functions without the ctx, for use in examples, and very simple use cases, and so on:

// (simplified)

// in the standard package
function Resolve(ctx context.Context, p path.Path) (*merkledag.Node, error) { ... }

// in a separate (simplifying) package
function Resolve(p path.Path) (*merkledag.Node, error) {
  ctx, cancel := context.WithCancel(n.Context())
  defer cancel()
  return core.Resolve(ctx, p)
}

whyrusleeping · 2015-05-03T21:25:42Z

One way is to have a package that mirrors the functions without the ctx

I'm not a fan of expanding the surface area of our exported API. Ideally, we have as few top level functions as possible, to reduce the mental load on users

cryptix · 2015-05-04T00:26:52Z

Addressed comments and rebased

whyrusleeping · 2015-05-04T05:31:14Z

commands/http/handler.go

+
+		I assume it really isn't because ServeHTTP() doesn't take a pointer receiver, but it's really subtule..
+
+		Shouldn't the context be just put on the command request?


Yeah, this is super sketchy. I think its probably best to put the context into the request object instead of here on the handler context

well, it is put into the request object, but it's put here: line 104. maybe we can find another place to put it, earlier? (nbd for me -- i want to rewire how all this works already)

whyrusleeping · 2015-05-04T05:33:59Z

Looks good to me! its a bit cleaner now too, good job

jbenet · 2015-05-04T15:32:15Z

@whyrusleeping

I'm not a fan of expanding the surface area of our exported API. Ideally, we have as few top level functions as possible, to reduce the mental load on users

The problem is that many times the type system and many knobs to use are a mental load on users already. Understanding + using contexts correctly is certainly a very big one.

And, it's a separate package, only there if you need it. It's not a bigger surface area on the same package-- it's tucked away. The functions aren't different-- it's a tweaked API for the same thing. Anyway, even if it doesn't go into go-ipfs, i'd make this a package and publish it somewhere.

If modules are very simple, and stable-- more modules isn't bad. Bigger modules with lots of semantics are painful.

jbenet · 2015-05-04T15:34:32Z

This LGTM.

jbenet · 2015-05-04T15:35:27Z

Didn't pass on any osx-- dont seem related but running again just in case.

commands/object: remove objectData() and objectLinks() helpers resolver: added context parameters sharness: $HASH carried the \r from the http protocol with sharness: write curl output to individual files http gw: break PUT handler until PR#1191

jbenet · 2015-05-08T05:33:48Z

@cryptix 👏 👏 👏 👏 👏 👏 very clean PR!

core: add context.Context param to core.Resolve()

jbenet added the status/in-progress In progress label May 3, 2015

cryptix reviewed May 3, 2015
View reviewed changes

cryptix mentioned this pull request May 3, 2015

Refactor/http gateway #1191

Merged

whyrusleeping reviewed May 3, 2015
View reviewed changes

cryptix force-pushed the refactor/coreResolve branch from 1db73d8 to 72ee5d5 Compare May 3, 2015 11:59

jbenet reviewed May 3, 2015
View reviewed changes

cryptix force-pushed the refactor/coreResolve branch from 6a9be55 to 274da9e Compare May 4, 2015 00:26

whyrusleeping reviewed May 4, 2015
View reviewed changes

cryptix force-pushed the refactor/coreResolve branch from 274da9e to 44d47af Compare May 4, 2015 13:21

cryptix force-pushed the refactor/coreResolve branch from 44d47af to 133fedd Compare May 8, 2015 00:12

cryptix force-pushed the refactor/coreResolve branch from 3791071 to f640ba0 Compare May 8, 2015 01:15

jbenet added a commit that referenced this pull request May 8, 2015

Merge pull request #1189 from ipfs/refactor/coreResolve

cd37b67

core: add context.Context param to core.Resolve()

jbenet merged commit cd37b67 into master May 8, 2015

jbenet deleted the refactor/coreResolve branch May 8, 2015 05:33

jbenet removed the status/in-progress In progress label May 8, 2015

This was referenced May 8, 2015

Rework mutable namespace resolution to handle recursion #1208

Merged

Sprint F ipfs/team-mgmt#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add context.Context param to core.Resolve() #1189

core: add context.Context param to core.Resolve() #1189

cryptix commented May 3, 2015

cryptix May 3, 2015

whyrusleeping May 3, 2015

whyrusleeping May 3, 2015

cryptix May 3, 2015

jbenet May 3, 2015

jbenet May 3, 2015

cryptix May 3, 2015

jbenet May 4, 2015

jbenet May 3, 2015

cryptix May 3, 2015

jbenet commented May 3, 2015

whyrusleeping commented May 3, 2015

cryptix commented May 4, 2015

whyrusleeping May 4, 2015

jbenet May 4, 2015

jbenet May 4, 2015

whyrusleeping commented May 4, 2015

jbenet commented May 4, 2015

jbenet commented May 4, 2015

jbenet commented May 4, 2015

jbenet commented May 8, 2015


		I assume it really isn't because ServeHTTP() doesn't take a pointer receiver, but it's really subtule..

		Shouldn't the context be just put on the command request?

core: add context.Context param to core.Resolve() #1189

core: add context.Context param to core.Resolve() #1189

Conversation

cryptix commented May 3, 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

Choose a reason for hiding this comment

jbenet commented May 3, 2015

whyrusleeping commented May 3, 2015

cryptix commented May 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented May 4, 2015

jbenet commented May 4, 2015

jbenet commented May 4, 2015

jbenet commented May 4, 2015

jbenet commented May 8, 2015