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

Add gRPC tests for trailers-only #904

Merged
merged 1 commit into from
Aug 2, 2024
Merged

Conversation

jhump
Copy link
Member

@jhump jhump commented Aug 2, 2024

This mirrors the gRPC-Web test cases that also handle different forms of trailers in responses.

These weren't originally included because I didn't think they would work: I thought that the "net/http" server would encode these on the wire in HTTP/2 using a HEADER frame (without END_STREAM flag) followed by an empty DATA frame (with END_STREAM flag). But the gRPC spec (and Google's implementations) require a trailers-only response in HTTP/2 be a single HEADER frame with END_STREAM flag set.

However, I was mistaken: the Go "net/http" server does exactly what we need it to. It would only use two frames (i.e. a second, empty DATA frame) if the handler flushes the http.ResponseWriter before it returns. So I didn't have to actually touch the reference server for this to work. 🎉

@jhump jhump requested a review from smaye81 August 2, 2024 04:14
@jhump
Copy link
Member Author

jhump commented Aug 2, 2024

cc @timostamm

@jhump jhump force-pushed the jh/support-trailers-only-in-ref-svr branch from a7b66bf to 2a9ee2c Compare August 2, 2024 04:15
@jhump jhump changed the title Adds gRPC tests for trailers-only Add gRPC tests for trailers-only Aug 2, 2024
@jhump jhump merged commit ad21d7f into main Aug 2, 2024
7 of 8 checks passed
@jhump jhump deleted the jh/support-trailers-only-in-ref-svr branch August 2, 2024 14:42
@jhump jhump mentioned this pull request Aug 13, 2024
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.

2 participants