Add timeout to Provide call #8
Conversation
|
@postables, @hsanjuan do you mind taking a look and reviewing this PR? |
michaelavila
force-pushed
the
fix/add-timeout-to-provide
branch
from
00c56a1
to
c2e647a
Compare
There was a problem hiding this comment.
LGTM but I would prefer this was configurable. 3 minute is a bit arbitrary (being that it's a timeout for a content routing implementation and we don't know what Provide involves in it).
I also don't know if setting a timeout here might have unwanted side-effects. If it's just working around a DHT bug, what happens when that is fixed? Will 3 minutes still be the ok value? Or will disabling the timeout altogether be the right approach because the DHT will timeout by itself? What I mean is that it should probably default to "no timeout" (current behaviour) and optionally be configurable to timeout.
In short, probably exporting ProvideTimeout as a variable with a 0 default meaning "no timeout" is enough.
michaelavila
force-pushed
the
fix/add-timeout-to-provide
branch
from
915c96c
to
94eb43f
Compare
michaelavila
force-pushed
the
fix/add-timeout-to-provide
branch
from
94eb43f
to
c3bccce
Compare
|
@hsanjuan that's all fair and good feedback, thanks. I took that into consideration and made some changes. Let me know what you think. |
simple/provider.go
Outdated
| go func() { | ||
| for p.ctx.Err() == nil { | ||
| select { | ||
| case <-p.ctx.Done(): | ||
| return | ||
| case c := <-p.queue.Dequeue(): | ||
| ctx, cancel := context.WithTimeout(p.ctx, p.timeout) |
There was a problem hiding this comment.
Right now default timeout is 0 and the context will be cancelled immediately.
There was a problem hiding this comment.
I suggest changing the mockRouting in the tests to fail if the context is expired.
There was a problem hiding this comment.
Yeah, I'll add a few tests around timing out.
michaelavila
force-pushed
the
fix/add-timeout-to-provide
branch
from
19db4ce
to
5999bf8
Compare
michaelavila
force-pushed
the
fix/add-timeout-to-provide
branch
from
5999bf8
to
5f6a572
Compare
|
I believe this will simply cause every single provide to timeout. Unfortunately, a timed-out provide is almost always completely useless (i.e., doesn't put anything in the DHT). |
michaelavila
changed the title
|
I have done some testing of this PR, the revision I'm using according to Findings are recorded in the corresponding github issue |
|
@Stebalien I think you're right and we just have some unexpected behavior, though I also think the issue has been around for a long time, which is why the timeout exists for the same call in |
|
Discussed in the standup, apparently we ignore the context error in That means we'll be able to send provider records as long as none of these requests block for too long (and check the context). As discussed, we should add a separate timeout (internally) on |
There was a problem hiding this comment.
We now have libp2p/go-libp2p-kad-dht#351 so LGTM modulo the defer issue.
simple/provider.go
Outdated
| var cancel context.CancelFunc | ||
| if p.timeout > 0 { | ||
| ctx, cancel = context.WithTimeout(p.ctx, p.timeout) | ||
| defer cancel() |
|
I believe all the issues have now been fixed. |
This just adds the the ability to set a timeout that is used by go-bitswap when doing a
Provide. Partially addresses #7.This also makes the worker count configurable.