-
Notifications
You must be signed in to change notification settings - Fork 27
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
send fewer packets by combining writes when possible #32
Conversation
CC @marten-seemann. This was inspired by libp2p/go-libp2p#413. |
client.go
Outdated
@@ -9,28 +10,52 @@ import ( | |||
// the protocol specified for the handshake. | |||
var ErrNotSupported = errors.New("protocol not supported") | |||
|
|||
// ErrNoProtocols is the error returned when the no protocols have been | |||
// specified. | |||
var ErrNoProtocols = errors.New("protocol not supported") |
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 should be difficult from ErrNotSupported
, otherwise ErrNotSupported == ErrNoProtocols
.
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.
(I should try to not copy and paste...)
c6b2b30
to
32ee2f5
Compare
if len(protos) == 0 { | ||
return "", ErrNoProtocols | ||
} | ||
switch err := SelectProtoOrFail(protos[0], rwc); 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.
why isn't this part of the loop below?
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.
I'm combining the /multistream/1.0
handshake when negotiating the first protocol. I'll leave a comment.
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.
Done. This should drop us from two round trips to 1.
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.
ok, good.
This makes the non-lazy negotiator act *slightly* more like the lazy one. Instead of sending the multistream header and *then* the protocol header, it sends them both in the same packet. This *shouldn't* cause any issues but we should still think carefully about whether or not this is worth it. It: 1. Sends fewer packets. 2. Saves half an RTT.
32ee2f5
to
bfe6132
Compare
This makes the non-lazy negotiator act slightly more like the lazy one.
Instead of sending the multistream header and then the protocol header, it
sends them both in the same packet.
This shouldn't cause any issues but we should still think carefully about
whether or not this is worth it. It: