Make sure ctx in commands are derived from req.Context #1585

rht · 2015-08-16T17:25:25Z

Fixes #505

rht · 2015-08-16T17:48:32Z

@whyrusleeping there are still several left, pending. And how to add a context to https://github.com/ipfs/go-ipfs/blob/master/core/coreunix/metadata_test.go#L52?

rht · 2015-08-16T18:09:47Z

Or keep the option that ipfsnode can be initialized without ctx. and modify (n *ipfsnode) Context() method to assign context.TODO() when n.ctx is nil

rht · 2015-08-16T18:27:04Z

unixfs/io/dirbuilder.go

@@ -29,22 +27,13 @@ func NewDirectory(dserv mdag.DAGService) *directoryBuilder {
 }

 // AddChild adds a (name, key)-pair to the root node.
-func (d *directoryBuilder) AddChild(name string, k key.Key) error {
-	// TODO(cryptix): consolidate context managment
-	ctx, cancel := context.WithTimeout(context.TODO(), time.Minute)


no sure if WithTimeout should be removed.
In other places sometimes it is 30 seconds or a minute, or no WithTimeout at all.

I there must be WithTimeout, then this should be inside dserv.Get

Ideally, most of the places we do this (context.TODO with a timeout) should be replaced with a passed in context. and the caller can decide the timeout if they want.

👍 to removing this timeout.

remove or replace with WithCancel?

whyrusleeping · 2015-08-16T18:48:37Z

One thing to note, we should try to avoid node.Context() if we can, as its the 'global' context. We should prefer a finer grained context when we can get it.

rht · 2015-08-16T19:56:32Z

though for now, node.Context() is more fine grained than context.tODO()

whyrusleeping · 2015-08-16T20:01:27Z

for sure! just wanted to make a note of that.

jbenet · 2015-08-17T02:28:20Z

unixfs/mod/dagmodifier.go

@@ -460,7 +460,7 @@ func dagTruncate(nd *mdag.Node, size uint64, ds mdag.DAGService) (*mdag.Node, er
 	var modified *mdag.Node
 	ndata := new(ft.FSNode)
 	for i, lnk := range nd.Links {
-		ctx, cancel := context.WithTimeout(context.TODO(), time.Minute)
+		_ctx, cancel := context.WithTimeout(ctx, time.Minute)


maybe this timeout one is one to remove?

We'd still want a context derived from the one passed in, so things can be cancelled when this func returns

GetNode basically calls DAG.Get, so WithCancel can be done inside DAG.Get too.
(ok but most are used for func-specific context)

jbenet · 2015-08-17T02:31:50Z

thanks for this PR, much needed cleanup

rht · 2015-08-17T08:16:21Z

WithTimeout cleanup should be in a separate PR.

rht · 2015-08-17T08:29:39Z

The remaining cleanup TODO has larger depths, and requires review:

p2p/host/basic/natmgr.go#L147: too buried down
eventlog: not sure if this needs cleanup
merkledag/traverse/traverse.go is not used
importer/helpers/helpers.go: too buried down

Won't touch:

test files
in exchange/bitswap/testnet/virtual.go func (n *bitswap.network) SendMessage because of the comment 'nb: terminate the context since the context wouldn't actually be passed over the network in a real scenario', so won't touch
routing/mock/dht.go mockrouting.NewDHTNetwork
in routing/supernode/proxy/loopback.go func (lb *Loopback) HandleStream(s inet.Stream), seems to be intentional
exchange/bitswap/testnet/peernet.go func (pn *bitswap.peernet) Adapter seems to be intentional
trickle.VerifyTrickleDagStructure only used in test (and tests already use context.TODO())

rht · 2015-08-17T09:02:23Z

WithTimeout-specific question. Should this be moved inside DAG.Get? While outside of it, also use WithCancel.

whyrusleeping · 2015-08-17T16:40:11Z

probably okay to leave for now
eventlog doesnt really use contexts for cancellation, so no need to worry about that
probably fine not to touch then
i think there is a context passed in by the dagbuilderhelper that you might have access to.

Thanks for doing all this, its really good stuff :)

rht · 2015-08-18T14:05:30Z

4 dagbuilderhelper doesn't have ctx by default. Should it have a ctx field?

whyrusleeping · 2015-08-19T17:33:39Z

@rht hrm.. i'm torn on this one. The only time a context is used is in a situtation where we are expected to already have the node local. We could wire a context down through there, but i dont think that its going to be worthwhile (i could be wrong though). What we could do is change the context.TODO() to a context.Background() and put a comment saying why

rht · 2015-08-20T00:53:21Z

Or just add ctx explicitly as an extra argument, here the ctx flow is:
DagModifier.ctx -> trickle.TrickleAppend -> (n *helpers.UnixfsNode) GetChild

rht · 2015-08-20T01:11:53Z

@whyrusleeping how about this?

Though most of the buried down, long context pipes are constructed because of the need to call dagservice GetNode / GetDAG. It is possibly much cleaner to hide the pipes inside dagservice? (will check if this is the case)

rht · 2015-08-20T01:32:02Z

why it is worthwhile: to make sure there are no 1-min zombie dagservices when an IpfsNode closes.

rht · 2015-08-20T01:44:09Z

p2p/host/basic/natmgr.go#L147: too buried down

This could be fixed with replacing L120 with worker.GoChild(func(p goprocess.Process) {addPortMapping(nmgr, addr)})?

Edit: i c, this natmgr.go#L147 is used for eventlog only, so it's fine.

rht · 2015-08-20T02:10:40Z

if err == nil { ready to merge }

whyrusleeping · 2015-08-20T06:15:22Z

LGTM

License: MIT Signed-off-by: rht <[email protected]>

rht · 2015-08-23T13:00:34Z

move ctx inside dag.Get? https://github.com/ipfs/go-ipfs/pull/1585/files#r37702188

I think this should be done in separate PR. Not every dag.Get caller waits only on this function to finish.

License: MIT Signed-off-by: rht <[email protected]>

rht · 2015-08-23T15:23:21Z

move ctx inside dag.Get? https://github.com/ipfs/go-ipfs/pull/1585/files#r37702188
I think this should be done in separate PR. Not every dag.Get caller waits only on this function to finish.

Added here anyway. I only made the change here when it is unambiguous that having a context.WithCancel before calling dag.Get makes it redundant AND when this ctx is not used again later on in the scope.

This also "auto-fix" parts of the code where there is no context.WithCancel before a dag.Get.

Instead put it inside of DAG.Get. The fix is applied only in the case when the context.WithCancel before a DAG.Get is also used later on in the scope. License: MIT Signed-off-by: rht <[email protected]>

jbenet · 2015-08-23T18:24:33Z

This means all lines matching 'ctx, _ *' are to be replaced with 'ctx, cancel ' + defer cancel()?

yeah.

jbenet · 2015-08-23T18:29:51Z

namesys/publisher.go

@@ -60,7 +60,8 @@ func (p *ipnsPublisher) Publish(ctx context.Context, k ci.PrivKey, value path.Pa

 	log.Debugf("Storing pubkey at: %s", namekey)
 	// Store associated public key
-	timectx, _ := context.WithDeadline(ctx, time.Now().Add(time.Second*10))
+	timectx, cancel := context.WithDeadline(ctx, time.Now().Add(time.Second*10))
+	defer cancel()


we can probably make this a constant too (and possibly even remove it??)

maybe a future PR can go over all the timeouts and make sure they make sense

Better in another PR. This PR is specifically to make sure all context trees are linked to the root caller.

sounds good 👍

jbenet · 2015-08-23T18:31:33Z

Ok, this LGTM. the only remaining things:

do all commits pass? (maybe run gpe? or squash them if too annoying/slow?)
is @whyrusleeping ok with merging this, given dev 0.4.0 and rebase ugliness?

maybe let's make all further refactor/cleanup happen on top of 0.4.0

jbenet · 2015-08-23T18:32:08Z

thanks @rht , this will improve codebase (+ perf for users in slow networkes!) a lot!

rht · 2015-08-23T23:02:56Z

(+ perf for users in slow networkes!)

in what way?

jbenet · 2015-08-23T23:07:47Z

requests wont timeout prematurely

rht · 2015-08-23T23:24:57Z

if slow means high latency: lunar RTT is about 2.5s, mars RTT is about 500s-2500s.

rht · 2015-08-23T23:32:04Z

gpe (on rht/go-ipfs) doesn't run the test unless it is done on ipfs/go-ipfs itself.

jbenet · 2015-08-23T23:49:43Z

@rht you have push access to this repo directly. you can push branches here

rht · 2015-08-24T03:30:48Z

gpe-ed the commits
Just tested a dev0.40 rebase against this branch. There are merge conflicts due to commits for dagmodifier/ipfs add (already in master), but not from this branch.

jbenet · 2015-08-24T08:33:32Z

Ok, sounds good! thanks @rht

Make sure ctx in commands are derived from req.Context

Make sure ctx in commands are derived from req.Context This commit was moved from ipfs/kubo@a965316

jbenet added the status/in-progress In progress label Aug 16, 2015

rht force-pushed the cleanup-context branch from 24ee3df to 0e96262 Compare August 16, 2015 18:13

rht reviewed Aug 16, 2015
View reviewed changes

jbenet reviewed Aug 17, 2015
View reviewed changes

rht force-pushed the cleanup-context branch 2 times, most recently from 93fd7f0 to 7ecbdbd Compare August 17, 2015 08:25

rht force-pushed the cleanup-context branch from 68c51b8 to 70e8427 Compare August 20, 2015 01:08

rht added the ready to merge label Aug 20, 2015

jbenet mentioned this pull request Aug 22, 2015

Sprint Aug 17 ipfs/team-mgmt#28

Closed

16 tasks

rht added 3 commits August 23, 2015 19:35

Wire a context down to (n *helpers.UnixfsNode) GetChild

34e06f6

License: MIT Signed-off-by: rht <[email protected]>

Fix 'ctx, _' to have explicit cancel

a7202fa

License: MIT Signed-off-by: rht <[email protected]>

Define discoveryConnTimeout and set it to 30s

fcf915f

License: MIT Signed-off-by: rht <[email protected]>

rht force-pushed the cleanup-context branch from 70e8427 to fcf915f Compare August 23, 2015 12:51

Replace context.TODO in test files with context.Background

30e03b2

License: MIT Signed-off-by: rht <[email protected]>

Remove ctx timeout in unixfsNode GetChild

dc21535

License: MIT Signed-off-by: rht <[email protected]>

rht force-pushed the cleanup-context branch from fb89bf2 to dc21535 Compare August 23, 2015 14:27

rht added codereview and removed ready to merge labels Aug 23, 2015

Fix ctx used in pub.Publish

ccd06e5

License: MIT Signed-off-by: rht <[email protected]>

Localize the scope of context.WithCancel for every DAG.Get

de5c0ce

Instead put it inside of DAG.Get. The fix is applied only in the case when the context.WithCancel before a DAG.Get is also used later on in the scope. License: MIT Signed-off-by: rht <[email protected]>

rht force-pushed the cleanup-context branch from 4f2939b to de5c0ce Compare August 23, 2015 15:38

jbenet reviewed Aug 23, 2015
View reviewed changes

jbenet added a commit that referenced this pull request Aug 24, 2015

Merge pull request #1585 from rht/cleanup-context

a965316

Make sure ctx in commands are derived from req.Context

jbenet merged commit a965316 into ipfs:master Aug 24, 2015

jbenet removed codereview status/in-progress In progress labels Aug 24, 2015

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#1585 from rht/cleanup-context

46792a3

Make sure ctx in commands are derived from req.Context This commit was moved from ipfs/kubo@a965316

Make sure ctx in commands are derived from req.Context #1585

Make sure ctx in commands are derived from req.Context #1585

Conversation

rht commented Aug 16, 2015

rht commented Aug 16, 2015

rht commented Aug 16, 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

whyrusleeping commented Aug 16, 2015

rht commented Aug 16, 2015

whyrusleeping commented Aug 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Aug 17, 2015

rht commented Aug 17, 2015

rht commented Aug 17, 2015

rht commented Aug 17, 2015

whyrusleeping commented Aug 17, 2015

rht commented Aug 18, 2015

whyrusleeping commented Aug 19, 2015

rht commented Aug 20, 2015

rht commented Aug 20, 2015

rht commented Aug 20, 2015

rht commented Aug 20, 2015

rht commented Aug 20, 2015

whyrusleeping commented Aug 20, 2015

rht commented Aug 23, 2015

rht commented Aug 23, 2015

jbenet commented Aug 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Aug 23, 2015

jbenet commented Aug 23, 2015

rht commented Aug 23, 2015

jbenet commented Aug 23, 2015

rht commented Aug 23, 2015

rht commented Aug 23, 2015

jbenet commented Aug 23, 2015

rht commented Aug 24, 2015

jbenet commented Aug 24, 2015