[plaintext] Retain remaining read buffer after handshake. #1831
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When the plaintext protocol handshake finishes and the `Framed` I/O is discarded in favour of the underlying I/O stream, the remaining read buffer of `Framed` must be retained, as it may have buffered data beyond the end of the handshake.
romanb
commented
Nov 10, 2020
| // was already received but is no longer part of the plaintext | ||
| // handshake. We need to capture that data before dropping | ||
| // the `Framed` wrapper via `Framed::into_inner()`. | ||
| let read_buffer = framed_socket.read_buffer().clone().freeze(); |
Contributor
Author
There was a problem hiding this comment.
We may be able to avoid cloning here in the future, if something like matthunz/futures-codec#54 is available.
mxinden
approved these changes
Nov 11, 2020
Member
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Well spotted. Thanks for fixing this!
Contributor
Author
|
Thanks for the quick review. Any objections if I make a patch release straight away? |
Member
@romanb patch release sounds good to me. Thanks! |
Contributor
Author
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the plaintext protocol handshake finishes and the
FramedI/O used during the handshake is discarded in favor of the underlying I/O stream, the remaining read buffer ofFramedmust be retained, as it may have buffered data beyond the end of the handshake. This problem manifests itself in potential stalls / timeouts during protocol negotiation, because a peer does not necessarily wait for confirmation of a protocol before it sends the proposal for the next protocol upgrade. E.g./multistream/1.0.0 \n /mplex/6.7.0for the mplex upgrade may already be received and buffered by theFramedof the plaintext handshake by a peer before the handshake is complete. That peer will read the final handshake frame but then discard the remaining read buffer ofFramed, leaving the remote waiting for the protocol confirmation for the next upgrade, but that proposal got "lost". The problem first appeared in #1765 where it was missed during review.This surfaced, without looking for it, during some benchmark attempts with the TCP transport and plaintext protocol in order to settle #802. However, because the symptoms are the same, I'm reasonably certain that this is the actual cause of the early connection negotiation stalls observed in #1629 (comment), unrelated to the random upgrade timeouts (which should already be fixed), since that test setup intentionally uses the plaintext protocol. So hopefully this should resolve the remaining problem(s) mentioned in #1629 that I had trouble to reproduce earlier.