-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Localize WithCancel contexts and remove DAGService timeout #1608
Conversation
@@ -174,6 +175,8 @@ func (h *BasicHost) NewStream(pid protocol.ID, p peer.ID) (inet.Stream, error) { | |||
// h.Network.Dial, and block until a connection is open, or an error is | |||
// returned. // TODO: Relay + NAT. | |||
func (h *BasicHost) Connect(ctx context.Context, pi peer.PeerInfo) error { | |||
ctx, cancel := context.WithTimeout(ctx, host.DiscoveryConnTimeout) | |||
defer cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure we dont want to be able to specify our own timeout for this? this feels like the opposite of what we want to do...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far in the code the timeouts either have the same value, or none at all.
Rather than having to reason the timeout each time, why not localize to the place where the ctx and timeout are needed and specify the timeout here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because what if i know that i'm going to connect to a peer that has insane latency? with this change, i will have no way of specifying that we should wait longer for the connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, i think @whyrusleeping's right.
the timeout was originally for "discovered" peers, right? (not peers coming from an api consumer/client) maybe it makes sense for it to be just there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case then shouldn't all the timeouts in the code that use (rh *RoutedHost) Connect
be modified for the latency anyway? Not just for (n *IpfsNode) HandlePeerFOund
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the basic UX is:
- when calling Connect, client specifies timeout
- when connecting automatically from a discovered peer, use the
discoveryConnTimeout
b2ee5b5
to
065e97c
Compare
|
||
ctxT, _ = context.WithTimeout(ctx, time.Second*2) | ||
ctxT, _ := context.WithTimeout(ctx, time.Second*2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defer cancel()
f594ac2
to
c83e82f
Compare
Localized |
@@ -136,10 +136,10 @@ func TestValueGetSet(t *testing.T) { | |||
|
|||
connect(t, ctx, dhtA, dhtB) | |||
|
|||
ctxT, _ := context.WithTimeout(ctx, time.Second) | |||
dhtA.PutValue(ctxT, "/v/hello", []byte("world")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this timeout? we expect this to happen immediately, if it hangs, then thats wrong and we want to know about it.
License: MIT Signed-off-by: rht <rhtbot@gmail.com>
License: MIT Signed-off-by: rht <rhtbot@gmail.com>
Rebased. |
@@ -338,10 +338,8 @@ func (i *gatewayHandler) putHandler(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
tctx, cancel := context.WithTimeout(ctx, time.Minute) | |||
defer cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure this is a bad context to have-- what is the timeout on ctx
as is? is there one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we should keep this one. anything on the gateway should have an explicit timeout (although, maybe it should be made configurable)
The cancels on tests are going to help our tests go faster 👍 👍 |
Many of these removed contexts (particularly the @whyrusleeping PTAL too? |
closing, we can pull relevant changes out if needed. the rest will be difficult to rebase |
No description provided.