Drain req.body on server stream methods with no body defined #5240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paskozdilar
changed the title
johanbrandhorst
requested changes
Feb 14, 2025
There was a problem hiding this comment.
Thanks for this! Any chance you could add an integration test that was broken before this? See https://github.com/grpc-ecosystem/grpc-gateway/blob/main/examples/internal/integration/integration_test.go for the existing integration tests. You may need to add some more definitions to the test protos.
|
I take it you're working on those tests I requested, but also just letting you know that you'll need to regenerate the proto files. See CONTRIBUTING.md for steps. |
|
Since this PR solves a server problem, not a client one, integration tests need to track whether server context is closed when the request fails. That doesn't fit well in the current stucture of the tests. I'm trying to think of an elegant solution that doesn't involve forwarding a value through RunMain and server.Run. |
|
Yeah, seems like |
|
Alright, I added a special case in gateway setup for "/rpc/no-body/" methods just to remove the logger middleware, and now it seems to work fine. Test on main: Test on my branch: You can find the failing test on branch main-failing-test in my repository. Please let me know if this is acceptable, or if there's anything you'd prefer to be done differently. |
paskozdilar
force-pushed
the
main
branch
2 times, most recently
from
27cb4b6 to
f3ca564
Compare
johanbrandhorst
requested changes
Feb 18, 2025
|
Done, should have no lint warnings anymore. |
|
🤔 looks like Bazel failed to compile, that's a bit odd since the non-bazel build and generate job succeeded. |
|
Unfortunately, I'm not experienced with Bazel at all. These logs make me thing it's some kind of caching issue: I'll try to get into Bazel to maybe figure out stuff, but considering 1) my lack of experience with Bazel 2) my complete ignorance of the build environment, I wouldn't hope for much. |
johanbrandhorst
requested changes
Feb 19, 2025
|
Nope, it seems that Bazel failes on that exact line: Reverting the commit. |
|
I've invited you as a collaborator so you can push directly on my fork if you wish so. |
|
I've found a way to reproduce the CI fail locally: #!/usr/bin/env bash
cd $(dirname $0)
docker run -v $(pwd):/grpc-gateway -v $(pwd)/certs:/etc/ssl/certs -w /grpc-gateway --rm ghcr.io/grpc-ecosystem/grpc-gateway/build-env:1.23 \
/bin/bash -c 'make install && \
make clean && \
make generate'
docker run -itv $(pwd):/grpc-gateway -v $(pwd)/certs:/etc/ssl/certs -w /grpc-gateway --entrypoint /bin/bash --rm \
ghcr.io/grpc-ecosystem/grpc-gateway/build-env:1.23 -c '\
bazel run :gazelle -- update-repos -from_file=go.mod -to_macro=repositories.bzl%go_repositories && \
bazel run :gazelle && \
bazel run :buildifier &&
bazel test //...'It prints the same error. |
|
Here's the full error, printed when |
Comment on lines
+21
to
+22
| # gazelle:exclude no_body_post.pb.gw.go | ||
| # gazelle:exclude no_body_post_grpc.pb.go |
There was a problem hiding this comment.
I think this was what was missing, it passes locally for me now
johanbrandhorst
approved these changes
Feb 22, 2025
This will prevent connection leaking when client sends a body on a method with no body defined. It will cause hanging when client sends a body stream, but such calls will hang anyway, even without the fix. Fixes grpc-ecosystem#5236.
Co-authored-by: Johan Brandhorst-Satzkorn <[email protected]>
This reverts commit 749f977.
johanbrandhorst
force-pushed
the
main
branch
from
bc9610d to
a51a99a
Compare
|
Just did a rebase to fix a conflict |
|
Thanks for your contribution and for your patience with the CI checks :)! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will prevent connection leaking when client sends a body on a method with no body defined. It will cause hanging when client sends a body stream, but such calls will hang anyway, even without the fix.
Fixes #5236.
References to other Issues or PRs
#5236
Have you read the Contributing Guidelines?
Yes.
Brief description of what is fixed or changed
When an HTTP client send body on a method which doesn't have (google.api.http) { body } annotation defined, grpc-gateway never closes context, causing connection leaks.
This PR drains req.Body on every method which doesn't have body annotation defined, so methods will work even if client sends body.
Other comments