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

Capture unexpected io.EOF errors as io.ErrUnexpectedEOF #533

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Jun 27, 2023

On Unmarshal we return io.EOF if there is no envelope header. The errSpecialEnvelope wraps io.EOF and is used in client code to check for stream errors. To distinguish between them we can instead check on Receive streams if the error is not a known errSpecialEnvelope and return an io.ErrUnexpectedEOF.

Fixes https://github.com/bufbuild/connect-go/issues/397

@emcfarlane emcfarlane self-assigned this Jun 27, 2023
@emcfarlane emcfarlane force-pushed the emcfarlane/unexpected-eof branch from 929c254 to dc18bf1 Compare June 27, 2023 13:13
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.

LGTM!

Do we have test cases today that cover other early-EOF scenarios? e.g. what about if we stop part-way through a frame?

@emcfarlane
Copy link
Contributor Author

@jchadwick-buf we do handle partial frames, this seems like an issue with request streams being able to close without a flag message but response streams should always have a final flagged end response.

@emcfarlane
Copy link
Contributor Author

emcfarlane commented Jun 27, 2023

@jchadwick-buf not sure if we already have error testing around it but added two: partial message and partial frame. Fixed partial frame error message.

@emcfarlane emcfarlane force-pushed the emcfarlane/unexpected-eof branch from 67775d4 to 849635c Compare June 27, 2023 16:36
@emcfarlane emcfarlane merged commit 7406ad6 into main Jun 28, 2023
@emcfarlane emcfarlane deleted the emcfarlane/unexpected-eof branch June 28, 2023 15:19
@akshayjshah
Copy link
Member

Is the error code the same for all three RPC protocols? I think we'll end up with CodeInternal for gRPC and CodeUnknown for Connect. Can we expand the test coverage to include all three protocols?

emcfarlane referenced this pull request Jul 11, 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
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
Capture unexpected io.EOF errors as io.ErrUnexpectedEOF in client streams. Now raises an error when the stream doesn't end with an end response message.

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

Distinguish between "the server ended the stream" and "the connection dropped unexpectedly"
3 participants