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.

Sign up for GitHub

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

Jump to bottom

Discard body of body-less methods in background #5331

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Discard body of body-less methods in background #5331

wants to merge 4 commits into from

Conversation

paskozdilar
Copy link
Contributor

@paskozdilar paskozdilar commented Mar 6, 2025 •
edited
Loading

When io.Copy is called synchronously on HTTP request body, it stalls the clients who keep the request stream open.
This broke WebSocket gateway implementations which rely on Close detection to know when to terminate.

This PR runs the io.Copy method in the background.

References to other Issues or PRs

Fixes #5326.

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

Added go keyword in front of io.Copy call in the template when body annotation is not set.

Other comments

@paskozdilar paskozdilar mentioned this pull request Mar 6, 2025
Open
@paskozdilar
Copy link
Contributor Author

Forgot to regenerate files. Doing it now.

@paskozdilar
Copy link
Contributor Author

Missing import for sync.WaitGroup.

I don't feel like tinkering with the import machinery - and benchmarks don't show any significant slowdown when using channels instead.

I'll update the implementation to use plain channels.

@paskozdilar
Copy link
Contributor Author

I'll try to write an integration test for this too.

@paskozdilar
Copy link
Contributor Author

paskozdilar commented Mar 7, 2025 •
edited
Loading

Apparently, transactional request-response flow is inherent to Go HTTP, including grpc-gateway runtime.
As such, it is impossible to "stream" data via HTTP client as we would with a WebSocket wrapper, so it's impossible to write an integration test for this without implementing a WebSocket gateway inside the server.

@johanbrandhorst
Would it be acceptable to add a minimal WebSocket wrapper inside the integration test server?

@johanbrandhorst
Copy link
Collaborator

Sure, it'd be great to know if we break the websocket proxy again in the future, why not!

@paskozdilar
Copy link
Contributor Author

I've opened a repository for the websocket proxy itself:
https://github.com/paskozdilar/grpc-gateway-websocket

It's fairly covered with tests, which fail on io.Copy and pass on go io.Copy.
I'll use this code as reference.

@paskozdilar
Copy link
Contributor Author

Please see: #5338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanbrandhorst johanbrandhorst johanbrandhorst left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v2.26.2 breaks grpc-websocket-proxy
2 participants
@paskozdilar @johanbrandhorst