Skip to content
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

Set sane default timeout for BasicHost.NewStream #3019

Closed
Tracked by #3005
MarcoPolo opened this issue Oct 23, 2024 · 0 comments · Fixed by #3020
Closed
Tracked by #3005

Set sane default timeout for BasicHost.NewStream #3019

MarcoPolo opened this issue Oct 23, 2024 · 0 comments · Fixed by #3020

Comments

@MarcoPolo
Copy link
Collaborator

MarcoPolo commented Oct 23, 2024

BasicHost.NewStream takes a context, but if that context is Background, it may block indefinitely. While it's better for the caller to set some timeout that makes sense in their context, we can do better than blocking forever.

Also see: libp2p/go-libp2p-kad-dht#994

Note: we tried to fix this here: https://github.com/libp2p/go-libp2p/pull/2907/files. But this is still an issue when we negotiate MSS:

// Negotiate the protocol in the background, obeying the context.
var selected protocol.ID
errCh := make(chan error, 1)
go func() {
selected, err = msmux.SelectOneOf(pids, s)
errCh <- err
}()
select {
case err = <-errCh:
if err != nil {
return nil, fmt.Errorf("failed to negotiate protocol: %w", err)
}
case <-ctx.Done():
s.Reset()
// wait for `SelectOneOf` to error out because of resetting the stream.
<-errCh
return nil, fmt.Errorf("failed to negotiate protocol: %w", ctx.Err())
}
.

@MarcoPolo MarcoPolo mentioned this issue Oct 23, 2024
20 tasks
@MarcoPolo MarcoPolo changed the title Set sane default timeout for NewStream Set sane default timeout for BasicHost.NewStream Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant