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

Allow deframer errors to close stream with a status code #11073

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

ryanpbrewster
Copy link
Contributor

Relevant to #3996

Today, if the deframer encounters an error it will trigger a stream reset. This does not provide the peer with much information about why the stream was reset. In particular, if a client sends an oversized message, the server will
reject the request, but the client will see that as a CANCELLED status code rather than a RESOURCE_EXHAUSTED.

grpc-go servers do provide this information to clients, and I think that's helpful.

This PR is an attempt to provide best-effort status codes to peers on deframer errors. @ejona86 explained (here) that it will not always be possible for the stream to gracefully close with properly encoded trailers. In these cases the peer will see garbled data. But in some circumstances — including the very prevalent case of a unary request payload that is too large — the status will be well-formed and useful.

Copy link

linux-foundation-easycla bot commented Apr 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ryanpbrewster / name: Ryan Brewster (fc49d80)

@ryanpbrewster
Copy link
Contributor Author

Tests & linters finally seem like they're passing.

@ejona86 can you recommend someone who could review this PR and help audit for other parts of the Netty implementation that need to be adjusted?

@ryanpbrewster
Copy link
Contributor Author

I missed the bit in CONTRIBUTING.md about maintaining a clean commit history. Tidied this up.

This change did not affect any existing tests. I added a new test specifically for this behavior, but it's fairly low-level. I tested manually this end-to-end by sending an oversized request and validating that I now get a RESOURCE_EXHAUSTED instead of a CANCEL

@ryanpbrewster
Copy link
Contributor Author

I think that :grpc-servlet:undertowTest is flaky, that test seems unrelated to any of the changes in this PR, and it passes locally.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

This looks good, with one null check needed

netty/src/main/java/io/grpc/netty/NettyServerHandler.java Outdated Show resolved Hide resolved
@ejona86
Copy link
Member

ejona86 commented Apr 24, 2024

I think that :grpc-servlet:undertowTest is flaky

Yeah, don't worry about it.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Thank you!

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
@ejona86
Copy link
Member

ejona86 commented Apr 24, 2024

Easy build failure:

Line is longer than 100 characters

@ryanpbrewster
Copy link
Contributor Author

Someday I'll push a commit that doesn't have a formatting error, but apparently not today :)

@ryanpbrewster
Copy link
Contributor Author

I'm used to squash-merging. Anything I need to do on my end to get this PR merged?

@ejona86
Copy link
Member

ejona86 commented Apr 24, 2024

We would squash when we merge. But you can squash yourself if you want more control of the commit message. (And this is a fine time to squash, as it won't make the review harder.) Looks like the commit message on your first commit is still accurate. I would probably end up prefixing the git commit title with "netty: " and append Fixes #3996. But that's minor stuff.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
Today, deframer errors cancel the stream without communicating a status code
to the peer. This change causes deframer errors to trigger a best-effort
attempt to send trailers with a status code so that the peer understands
why the stream is being closed.

Fixes grpc#3996
@ryanpbrewster
Copy link
Contributor Author

Squashed everything down and made the formatting suggestions 👍

@larry-safran larry-safran added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 24, 2024
@ejona86 ejona86 merged commit e036b1b into grpc:master Apr 24, 2024
15 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants