search

connectrpc / conformance

Conformance test suite for Connect, gRPC, and gRPC-Web implementations.
https://connectrpc.com
Apache License 2.0
56 stars 6 forks source link

Server-side implementation of FailServerStreaming is incorrect #384

Closed akshayjshah closed 1 year ago

akshayjshah commented 1 year ago

https://github.com/bufbuild/connect-crosstest/blob/3ceac5de26818d775fd345ea33da1b5093120072/internal/interop/interopconnect/test_server.go#L121 isn't quite right - we're supposed to be sending one message and then an error. Instead, we're sending an error immediately.

This is important, because the two behaviors exercise different code paths: sending an error right away lets the server send a trailers-only response (which is confusingly just HTTP headers with no body). Sending a message and then an error forces the server to encode the trailing metadata (including all the error information) at the end of the body. grpc-web treats header keys as case-insensitive in the first case but case-sensitive in the second case.

I believe that this has allowed a protocol bug to sneak into connect-go - we're not lowercasing HTTP/1.1 header and trailer names when speaking the gRPC-Web protocol. I've got an issue open on the spec to clarify expectations here, but we should update this test case to correctly expose the incompatibility.

@joshcarp can you take a look at this?

timostamm commented 1 year ago

@joshcarp, https://github.com/bufbuild/connect-crosstest/pull/386 covers both code paths described by Akshay for web clients. Go clients are only tested for trailers-only.