Netty server poorly handles unknown content type

[ejona86]

If the content-type is missing or unknown, the Netty transport sends RST_STREAM with REFUSED_STREAM. That's bonkers on multiple levels.

We should probably respond with HTTP 415 instead.

[lbensaad]

I was about to open an issue about this. I want a grpc-java server to serve a grpc-web client.
When the server receives a connection, it gives an exception:
SEVERE: Transport failed io.netty.handler.codec.http2.Http2Exception: Unexpected HTTP/1.x request

I want to write a wrapper the same way as in grpc-web wrapper.go

So I think the only way to add it, is to expose the netty pipeline and add a codec that transforms requests and responses between grpc and grpc-web.

If there is another way to do it please let me know.

[ejona86]

@lbensaad, your problem seems unrelated. Note that the older HTTP/2 API in Netty that gRPC is using doesn't let you "add a codec that transforms": it's monolithic and avoids the pipeline. gRPC needs to migrate to "h2childchan": netty/netty#3667

[lbensaad]

Thanks @ejona86.
Should I open another issue on how to do this wrapper?

[ejona86]

@lbensaad, I feel like we had an issue for that already, but I can't find it. Yeah, open up a new issue.

[ramaraochavali]

@ejona86 are you actively working on this?

[ejona86]

On a recent plane flight I messed with it some in https://github.com/ejona86/grpc-java/tree/dont-misuse-refused . But I've not had time to circle back to it. I'd be fine if you wanted to pick it up (as it stands, I'll have to re-discover what my TODOs and FIXMEs mean 😄).

[ramaraochavali]

@ejona86 I looked at it and could not find what you meant by couple of TODOs though. Otherwise the implementation looks fine.

[ejona86]

FIXME: ejona86@ad93041#diff-dde99a3f12e631de9f1605b24abd9c2bR371
TODO: ejona86@ad93041#diff-dde99a3f12e631de9f1605b24abd9c2bR618
TODO: ejona86@ad93041#diff-dde99a3f12e631de9f1605b24abd9c2bR622

I'm pretty sure that some tests fail. I think the FIXME is because the HTTP code is wrong. There's probably a need for some added tests.

[ramaraochavali]

@ejona86 I got your code and built the complete thing.

• Fixed the FIXME - Changed the HTTP code to 405.

• Changed the TODO : I think the TODO is related to hard coded "4". I changed it to serialized.length/2. Is that what you are expecting?

• Did not understand the last TODO : check boolean - What do you intend to check here?

As expected, the contentType test headersWithInvalidContentTypeShouldFail along with couple of other tests failed. I am looking at fixing them

[ejona86]

@ramaraochavali
The first TODO probably was for double-checking the entire constructor. So the 'true' is for validate. It's conceivable we could get away without validating, but given it's an error path it's probably not worth the chances of getting it wrong (thus 'true' is good). So changing the 4 like you did SGTM to fix that TODO.

The last TODO was to see if false was the right thing to pass. And yes, it seems good to pass. So it's fine.

To check if you need more tests, you can run ./gradlew test jacocoTestReport from the netty directory and check the code coverage in build/reports/jacoco for the new conditions. (Relying heavily on code coverage is not always right, but I think it's reasonable here.)

[ramaraochavali]

@ejona86 Changed the code, Fixed the existing test cases and added a couple of more to cover the new conditions. The PR is here #3735. Please review

[ejona86]

Fixed by #3735