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

Fix RST_STREAM when stream closing #12

Merged
merged 1 commit into from
May 24, 2022

Conversation

psalin
Copy link
Collaborator

@psalin psalin commented Dec 30, 2021

Fixes an issue where a RST_STREAM message is sent each time a
stream is closed. As was the case with the code prior to hpack
dynamic table concurrency fixes, sending trailers in
half_closed_remote state should transfer the state to closed with
a timeout action.

This is an alternative solution to #7 that fixes tsloughter/grpcbox#51 the way it used to be before (https://github.com/tsloughter/chatterbox/pull/5/files#diff-d3c0445427befca915a9897b50d4d356e5e360fd3bac4f0a773a4dd639771b3dR573).

@fnchooft
Copy link

Validated that the fix works.
Easiest way I found:
in grpcbox-directory:

mkdir _checkouts
cd _checkouts
git clone https://github.com/psalin/chatterbox.git
cd chatterbox && git checkout fix_rst_stream

After that clean the _build-dir and recompile.

Check deps:
rebar3 tree

I was facing the same issue using the grpcui-go-package.

@tsloughter
Copy link
Owner

I assume the 0 is to force a state timeout, but it may be better to use next_event to trigger an action to happen after.

Fixes an issue where a RST_STREAM message is sent each time a
stream is closed. As was the case with the code prior to hpack
dynamic table concurrency fixes, sending trailers in
half_closed_remote state should transfer the state to closed with
a timeout action.
@psalin
Copy link
Collaborator Author

psalin commented Feb 7, 2022

The timeout 0 method is used throughout the file so maybe best to keep it in this commit for consistency and leave refactoring to use next_event to a separate task.

@tsloughter
Copy link
Owner

Yea, works for me, I guess I should merge this and make a release tomorrow.

@tsloughter tsloughter merged commit 0d062a4 into tsloughter:master May 24, 2022
@psalin psalin deleted the fix_rst_stream branch September 14, 2022 09:27
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.

Always receive "stream terminated by RST_STREAM with error code: STREAM_CLOSED" after grpc request
3 participants