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

Netty 4.1.75.Final HTTP/2 connection closures #8981

Closed
chemicL opened this issue Mar 10, 2022 · 4 comments · Fixed by #9004
Closed

Netty 4.1.75.Final HTTP/2 connection closures #8981

chemicL opened this issue Mar 10, 2022 · 4 comments · Fixed by #9004
Assignees
@ejona86
Labels
bug
Milestone
1.46

Comments

@chemicL
Copy link

chemicL commented Mar 10, 2022 •
edited
Loading

What version of gRPC-Java are you using?

1.46.0-SNAPSHOT

What is your environment?

macOS

What did you expect to see?

After bumping Netty from 4.1.72.Final to 4.1.75.Final I'd expect tests to pass.

What did you see instead?

Broken tests:

> Task :grpc-alts:test

io.grpc.alts.HandshakerServiceChannelTest > resource_lifecycleTwice FAILED
    io.grpc.StatusRuntimeException: INTERNAL: Connection closed after GOAWAY. HTTP/2 error code: COMPRESSION_ERROR, debug data: Use direct accessor methods for pseudo headers.
        at app//io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:271)
        at app//io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:252)
        at app//io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:164)
        at app//io.grpc.testing.protobuf.SimpleServiceGrpc$SimpleServiceBlockingStub.unaryRpc(SimpleServiceGrpc.java:355)
        at app//io.grpc.alts.HandshakerServiceChannelTest.doRpc(HandshakerServiceChannelTest.java:98)
        at app//io.grpc.alts.HandshakerServiceChannelTest.resource_lifecycleTwice(HandshakerServiceChannelTest.java:85)

io.grpc.alts.HandshakerServiceChannelTest > resource_works FAILED
    io.grpc.StatusRuntimeException: INTERNAL: Connection closed after GOAWAY. HTTP/2 error code: COMPRESSION_ERROR, debug data: Use direct accessor methods for pseudo headers.
        at app//io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:271)
        at app//io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:252)
        at app//io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:164)
        at app//io.grpc.testing.protobuf.SimpleServiceGrpc$SimpleServiceBlockingStub.unaryRpc(SimpleServiceGrpc.java:355)
        at app//io.grpc.alts.HandshakerServiceChannelTest.doRpc(HandshakerServiceChannelTest.java:98)
        at app//io.grpc.alts.HandshakerServiceChannelTest.resource_works(HandshakerServiceChannelTest.java:75)

Steps to reproduce the bug

Bump nettyVersion to 4.1.75.Final and run ./gradlew clean build.

Possible cause

GrpcHttp2RequestHeaders most probably contains incorrect implementation and unnecessary guards against accessing pseudo headers, which are not specified by Netty and are in fact used by the contains method.
@ejona86 ejona86 added the bug label Mar 10, 2022
@ejona86 ejona86 added this to the v1.46 milestone Mar 10, 2022
@ejona86
Copy link
Member

ejona86 commented Mar 10, 2022

Looks like add() was set up to handle pseudo headers, but not get/remove. getAll() would be easy to implement by just calling get() since we know there's only one. Not hard, just more code like addPseudoHeader()... and actually simpler because we don't have to have the condition that throws for each.

@ejona86 ejona86 self-assigned this Mar 10, 2022
@Scottmitch Scottmitch mentioned this issue Mar 11, 2022
Merged
@njhill
Copy link
Contributor

njhill commented Mar 11, 2022

@ejona86 actually only contains() is needed so could have a slightly simpler impl? and maybe the duplicate checks could now be removed from addPseudoHeader() since netty now also checks for this? (the reason that contains() is now called)

This was referenced Mar 12, 2022
Closed
Closed
@npepinpe npepinpe mentioned this issue Mar 15, 2022
Merged
@njhill njhill mentioned this issue Mar 19, 2022
Closed
@njhill
Copy link
Contributor

njhill commented Mar 19, 2022

@ejona86 I had a go at this in #9001

@davsclaus
Copy link

We have this problem too with Apache Camel and our camel-grpc component, downgrading to netty 4.1.72.Final works
https://issues.apache.org/jira/browse/CAMEL-17832

ejona86 added a commit to ejona86/grpc-java that referenced this issue Mar 21, 2022
@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
1e59f1a 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes grpc#8981
@ejona86 ejona86 mentioned this issue Mar 21, 2022
Merged
ejona86 added a commit that referenced this issue Mar 22, 2022
@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
4a0fe99 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes #8981
ejona86 added a commit to ejona86/grpc-java that referenced this issue Mar 22, 2022
@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
363b346 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes grpc#8981
ejona86 added a commit to ejona86/grpc-java that referenced this issue Mar 22, 2022
@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
b065719 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes grpc#8981
This was referenced Mar 22, 2022
Merged
Merged
ejona86 added a commit that referenced this issue Mar 23, 2022
@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
9536204 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes #8981
ejona86 added a commit that referenced this issue Mar 23, 2022
@ejona86
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
8bc9b3b 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes #8981
@vpanta vpanta mentioned this issue Mar 31, 2022
Closed
temawi pushed a commit to temawi/grpc-java that referenced this issue Apr 8, 2022
@ejona86 @temawi
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
616176c 
The previous code assumed that only gRPC would be using these methods.
But twice now Netty has made a change (generally relating to security)
that used a method for pseudo headers that previously wasn't supported.
Let's stop the whack-a-mole and just implement them all.

This restores compatibility with Netty 4.1.75.Final. Fixes grpc#8981
@emeroad emeroad mentioned this issue Apr 13, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ejona86 ejona86

Labels
bug
Projects
None yet
Milestone
1.46
4 participants
@davsclaus @chemicL @ejona86 @njhill