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

Avoid unnecessary empty data frames at end of stream #140

Conversation

FinleyMcIlwaine
Copy link
Collaborator

If a stream terminates with an empty data frame, we will send that empty data frame even if it is succeeded by trailers that are marked end of stream. That empty data frame is unnecessary. We should only send it if the payload length is non-zero and we are not sending trailers.

I discovered this after seeing too many empty data errors from terminating a server handling several concurrent streams with an exception.

Currently, if a stream terminates with an empty data frame, we will send that
empty data frame even if it is succeeded by trailers that are marked end of
stream. That empty data frame is unnecessary. We should only send it if the
payload length is non-zero and we are not sending trailers.
@kazu-yamamoto kazu-yamamoto self-requested a review August 6, 2024 23:11
@FinleyMcIlwaine
Copy link
Collaborator Author

I cannot reproduce the current test failure locally after running the test on a loop for a while, on my branch or on main. My guess is that it is a non-deterministic failure that is not associated with these changes.

Copy link
Owner

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

kazu-yamamoto added a commit that referenced this pull request Aug 8, 2024
@kazu-yamamoto
Copy link
Owner

It seems to me that your logic is correct.
So, rebased and merged.
Thanks.

@FinleyMcIlwaine
Copy link
Collaborator Author

@kazu-yamamoto Thanks for that! Also, we have another PR incoming soon, so don't worry about releasing these changes just yet.

@kazu-yamamoto
Copy link
Owner

The previous release was for me.
I needed to deploy the new ServerIO for my project.

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 this pull request may close these issues.

2 participants