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

gRPC Trailers-Only responses must be a single HEADERS frame #3152

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

mgodave
Copy link
Contributor

@mgodave mgodave commented Dec 20, 2024

Replaces #3148

Motivation

When a blocking gRPC endpoint with streaming response encounters an exception, it attempts to send Trailers-Only response (grpc-status is returned in headers rather than trailers). However, the HTTP/2 layer sends two frames instead of one: HEADERS with endStream=false and DATA with endStream=true. Official grpc-java implementation doesn't like that, it wants a single HEADERS frame with endStream=true for Trailers-Only responses.

Changes

I added a filter and a context key to signal when we have a case where Trailers-Only response is sent. The filter will aggregate the response stream, observe there are no trailers, drop them, and will only return a single HEADERS frame with endStream=true.
This behavior can be tested by ProtocolCompatibilityTest.unimplementedServiceError, we unskipped tests that were failing before this fix.

Results

Before Change:

OUTBOUND HEADERS: streamId=3 headers=GrpcHttp2OutboundHeaders[:authority: 127.0.0.1:62451, :path: /grpc.netty.Compat/clientStreamingCall, :method: POST, :scheme: http, content-type: application/grpc, te: trailers, user-agent: grpc-java-netty/1.64.1, grpc-accept-encoding: gzip] streamDependency=0 weight=16 exclusive=false padding=0 endStream=false
OUTBOUND DATA: streamId=3 padding=0 endStream=false length=7 bytes=00000000020801
OUTBOUND DATA: streamId=3 padding=0 endStream=true length=0 bytes=
INBOUND HEADERS: streamId=3 headers=GrpcHttp2ResponseHeaders[:status: 200, server: servicetalk-grpc/, content-type: application/grpc+proto, grpc-status: 12, grpc-message: Method grpc.netty.Compat/clientStreamingCall is unimplemented] padding=0 endStream=false
INBOUND DATA: streamId=3 padding=0 endStream=true length=0 bytes=

After Change:

OUTBOUND HEADERS: streamId=3 headers=GrpcHttp2OutboundHeaders[:authority: 127.0.0.1:62421, :path: /grpc.netty.Compat/clientStreamingCall, :method: POST, :scheme: http, content-type: application/grpc, te: trailers, user-agent: grpc-java-netty/1.64.1, grpc-accept-encoding: gzip] streamDependency=0 weight=16 exclusive=false padding=0 endStream=false
OUTBOUND DATA: streamId=3 padding=0 endStream=false length=7 bytes=00000000020801
OUTBOUND DATA: streamId=3 padding=0 endStream=true length=0 bytes=
INBOUND HEADERS: streamId=3 headers=GrpcHttp2ResponseHeaders[:status: 200, server: servicetalk-grpc/, content-type: application/grpc+proto, grpc-status: 12, grpc-message: Method grpc.netty.Compat/clientStreamingCall is unimplemented, content-length: 0] padding=0 endStream=true

@mgodave mgodave changed the title Add gRPC filter to optimize the blocking server in cases where streams result in an error New default gRPC filter to optimize blocking server error cases Dec 20, 2024
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

Glad that all tests passed 🎉

Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

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

LGTM!
Will wait until my PR is merged and then will merge yours on Monday

@idelpivnitskiy idelpivnitskiy force-pushed the drusek/extra-data-frame branch from 6c55492 to 2445b1e Compare December 23, 2024 18:25
@idelpivnitskiy idelpivnitskiy changed the title New default gRPC filter to optimize blocking server error cases All gRPC Trailers-Only responses must only have a single HEADERS frame Dec 23, 2024
@idelpivnitskiy idelpivnitskiy changed the title All gRPC Trailers-Only responses must only have a single HEADERS frame gRPC Trailers-Only responses must be a single HEADERS frame Dec 23, 2024
@idelpivnitskiy idelpivnitskiy merged commit 2dfacd1 into apple:main Dec 23, 2024
11 checks passed
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