prevent wantmanager from leaking goroutines (and memory) #1356
Conversation
|
There were the following issues with your Pull Request
Guidelines are available at: https://github.com/ipfs/community/blob/master/docs/commit-message.md Your feedback on GitCop is welcome on the following issue: ipfs/infra#23 This message was auto-generated by https://gitcop.com |
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
whyrusleeping
force-pushed
the
fix/bitswap-leak
branch
from
c6b9422
to
c5b40b3
Compare
|
this failure: https://travis-ci.org/ipfs/go-ipfs/jobs/66410801 (ive seen it before, its unrelated) worries me. I'm not sure what it means. |
| // this includes looking them up in the dht | ||
| // dialing them, and handshaking | ||
| conctx, cancel := context.WithTimeout(ctx, time.Minute) | ||
| defer cancel() |
There was a problem hiding this comment.
understood that it's important to have a timeout here, but 1min is way too short. 15-30min is more ok. these timeouts affect people in the worst of situations, where a pageload could take >5min. It's awful, yes, but that's the rest of the world. We must work there too.
There was a problem hiding this comment.
alright, ten minutes then? tcp times out at 5.
There was a problem hiding this comment.
On Thu, Jun 11, 2015 at 01:26:40PM -0700, Jeromy Johnson wrote:
+func (mq *msgQueue) doWork(ctx context.Context) {
- // allow a minute for connections
- // this includes looking them up in the dht
- // dialing them, and handshaking
- conctx, cancel := context.WithTimeout(ctx, time.Minute)
- defer cancel()
alright, ten minutes then? tcp times out at 5.
Do we need explicit sub-part timeouts at all? Can't the caller
specify their own timeout when they submit a task? Looking into the
context handling here, runQueue gets launched with one context, and it
passes that context down to doWork. But maybe the doWork context
should be a union of the runQueue context and a second context that
gets passed in via the channel? With a check in runQueue to see if
any context-related doWork errors were due to a msgQueue-context
timeout/cancel (in which msgQueue should close down) or the
channel-passed context (in which case pass that error back to the
caller?).
There was a problem hiding this comment.
Do we need explicit sub-part timeouts at all? Can't the caller
specify their own timeout when they submit a task?
we could if all the callers/roots had timeouts enforced somehow. we dont do this perfectly yet.
But maybe the doWork context
should be a union of the runQueue context and a second context that
gets passed in via the channel?
could use https://github.com/jbenet/go-context/blob/master/ext/dagctx.go for this.
whyrusleeping
force-pushed
the
fix/bitswap-leak
branch
2 times, most recently
from
9e8e6e5
to
e014a66
Compare
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
prevent wantmanager from leaking goroutines (and memory)
got tired of not being able to figure out the spdy problem. So I went to something that I could solve.
the context for the runQueue is actually just the context for the entire wantmanager. It never really gets cancelled before the daemon shuts down. I added a timeout to the Connect and Send calls within the runQueue to prevent this from happening.
This still leaves one problem unsolved, what do we do if we cannot connect to a given peer, but bitswap insists on sending them a message? (line 157)