Skip to content

Add MethodType and MethodRequestBody query parameters #41

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

Closed
wants to merge 0 commits into from

Conversation

@paskozdilar
Copy link
Author

Wait, this still doesn't fix the issue from grpc-ecosystem/grpc-gateway#5326...
Please hold until more info.

@paskozdilar paskozdilar changed the title Add buffer goroutine between conn read and request write Add MethodType and MethodRequestBody query parameters Mar 28, 2025
@paskozdilar
Copy link
Author

A few updates:


The root problem that we're trying to fix here is related to Unary and ServerStream methods (those where client only sends a single request):

  • HTTP servers expect client to send body and then close the request
  • when wrapping http.Handler in a manner wsproxy does, the request doesn't have to be closed
  • grpc-gateway only parses the part of request it is interested in - it doesn't check of excess data or drain the req.Body
  • fix (workaround?) was added in v2.26.2 (Drain req.body on server stream methods with no body defined grpc-ecosystem/grpc-gateway#5240) which drained req.Body to prevent context leak
  • since wsproxy doesn't close req.Body until the client WebSocket is closed, the req.Body drain blocked indefinitely!

Now, this cannot be solved in grpc-gateway alone. I've proposed a PR with request drain in a new goroutine, but johanbrandhorst was (justifiably) reluctant to add it.

Now, the real issue with WebSocket proxy is that it doesn't close the req.Body in Unary and ServerStream methods.
However, it also has to keep req.Body open for ClientStream and Bidirectional methods.

Additionally, when body is defined in proto, WebSocket must send the request body and then close, and if it is not, it must not send the request body, but close immediately.

So the only two solutions for that were:

  1. add codegen from proto to websocket proxy
  2. add query parameter for specifying method type and whether body should be sent or not.

This PR implements the solution 2.


In case this PR is accepted, the documentation should probably be changed to make sure users understand how to use these parameters.

This will also be a backwards-incompatible change, since not specifying methodType will not allow request to pass.
So we should probably raise the major version too?

@paskozdilar
Copy link
Author

Actually, if grpc-gateway adds metadata to response header, we could detect the method types dynamically and avoid these params altogether. Maybe even get rid of MethodOverrideParam!

Please hold until grpc-ecosystem/grpc-gateway#5331 is finished.

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.

1 participant