fix(tonic): flush accumulated ready messages when status received #1756
Conversation
This file contains 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
hyperium#1423 introduced logic to buffer multiple ready messages in order to amortize the cost of sends to the underlying transport. This also introduced a change in behavior for tonic in the following scenario: A stream of ready messages less than the yield threshold is trailed by a status. Previously the ready messages would all have been yielded from the stream and sent, followed by the status. After the change was introduced the status is yielded from the stream and sent but the accumulated ready messages in the buffer are never sent out. This change adjusts the logic to restore the previous behavior while still retaining the amoritization benefits. Namely it flushes the accumulated ready messages prior to yielding the status ensuring they are sent out from the stream in the order they are read.
|
This makes a lot of sense, thanks! |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#1423 introduced logic to buffer multiple ready messages in order to amortize the cost of sends to the underlying transport. This also introduced a change in behavior for tonic in the following scenario:
A stream of ready messages less than the yield threshold is trailed by a status. Previously the ready messages would all have been yielded from the stream and sent, followed by the status. After the change was introduced the status is yielded from the stream and sent but the accumulated ready messages in the buffer are never sent out.
This change adjusts the logic to restore the previous behavior while still retaining the amoritization benefits. Namely it flushes the accumulated ready messages prior to yielding the status ensuring they are sent out from the stream in the order they are read.
Motivation
When upgrading tonic versions in our application we noticed some of our existing integration tests started failing. The pattern in these failures was that they involved a server stream yielding a message which was followed shortly after by a status message, and the observed behavior was that the client only received a status message from the stream and not the message that was sent in the stream first. The intent is to fix this to unblock our upgrade of tonic and restore the previous behavior while still retaining the benefits of amortization introduced in #1423.
Solution
This change checks to see if there are any accumulated messages in the buffer after polling a status message from the underlying stream and yields them first. On the subsequent poll it will then yield the status message.