-
Notifications
You must be signed in to change notification settings - Fork 112
fix(network): impl: add timeout in newStreamToPeer call #477
Conversation
tctx, cancel := context.WithTimeout(ctx, connectTimeout) | ||
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.
Taken from:
go-bitswap/network/ipfs_impl.go
Lines 106 to 116 in a28f6eb
tctx, cancel := context.WithTimeout(ctx, s.opts.SendTimeout) | |
defer cancel() | |
if err := s.bsnet.ConnectTo(tctx, s.to); err != nil { | |
return nil, err | |
} | |
stream, err := s.bsnet.newStreamToPeer(tctx, s.to) | |
if err != nil { | |
return nil, err | |
} |
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.
To clarify, I'm not setting the timeout inside newStreamToPeer
or as an argument (like msgToStream
does) because the current pattern in Connect
is embedding the timeout externally in the context passed to newStreamToPeer
. But I can change both if we want to go in another direction.
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.
This is probably fine for now.
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.
It would be nice to eventually deduplicate our two different message sending functions... But whatever...
tctx, cancel := context.WithTimeout(ctx, connectTimeout) | ||
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.
This is probably fine for now.
…reamToPeer-with-timeout fix(network): impl: add timeout in newStreamToPeer call This commit was moved from ipfs/go-bitswap@c13e78b
See ipfs/kubo#7972 (comment) for background.
If there is a straightforward 10-line way to test this I can add it, otherwise I'll just leave the fix.