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

Drain stream and error on trailing data #536

Merged
merged 8 commits into from
Jul 11, 2023
Merged

Conversation

emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Jun 27, 2023

For connect and gRPC-web on receiving end of stream payloads ensure no extra data is written by draining the reader and erroring on more data.

Extension of #533

Fixes #427

@emcfarlane emcfarlane self-assigned this Jun 27, 2023
Base automatically changed from emcfarlane/unexpected-eof to main June 28, 2023 15:19
@emcfarlane emcfarlane force-pushed the emcfarlane/drain-eof branch from df2f8da to b73bc7f Compare June 28, 2023 15:36
@emcfarlane emcfarlane marked this pull request as ready for review June 28, 2023 15:41
Copy link
Contributor

@jchadwick-buf jchadwick-buf left a comment

Choose a reason for hiding this comment

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

Seems reasonable enough to me!

@emcfarlane
Copy link
Contributor Author

Block on https://github.com/bufbuild/connect-go/pull/539 need to expand these testcases for grpc and grpc-web.

connect_ext_test.go Outdated Show resolved Hide resolved
envelope.go Outdated Show resolved Hide resolved
protocol_grpc.go Show resolved Hide resolved
connect_ext_test.go Outdated Show resolved Hide resolved
Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

One test nit, otherwise LGTM! Please add the error checks, but then this is good to land from my perspective.

connect_ext_test.go Outdated Show resolved Hide resolved
@emcfarlane emcfarlane merged commit e3fcf6f into main Jul 11, 2023
@emcfarlane emcfarlane deleted the emcfarlane/drain-eof branch July 11, 2023 18:25
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
For connect and gRPC-web on receiving end of stream payloads ensure no
extra data is written by draining the reader and erroring on more data.

Extension of https://github.com/bufbuild/connect-go/pull/533

Fixes https://github.com/bufbuild/connect-go/issues/427
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.

client side of server streaming call does not always drain HTTP response body
3 participants