grpcweb-transport: fix handling responses with empty body and status in headers #331
Conversation
Fixed grpcweb-transport to handle responses with empty body and status header.
| } | ||
| }); | ||
|
|
||
| it('success if empty response body and OK status in headers', async () => { |
There was a problem hiding this comment.
According to https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses
Most responses are expected to have both headers and trailers but Trailers-Only is permitted for calls that produce an immediate error.
Thus I would expect any "Trailers-Only" response to throw an RpcError.
There was a problem hiding this comment.
This is of course logical for non OK status codes. But in practice, it will be strange if received code OK will be accepted as error on client side.
Maybe initially this case (sending from server only status OK with empty body in stream) is strange, because I think normally server should send something like NOT_FOUND code if there is nothing to send in stream.
But at the moment exactly this handling is implemented in the official implementation of the protocol in https://github.com/grpc/grpc-web
There was a problem hiding this comment.
That sentence in the spec is frustratingly ambiguous. And unfortunately none of the official implementation tests actually assert what should happen when getting an OK trailers-only unary RPC response: https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/test/generated_code_test.js
Though searching through issues on grpc/grpc I get the sense that it is not allowed for the server to send an OK status without a message in a unary RPC. From grpc/grpc-java#2785 (comment):
Unary must either error or have a message.
The server does not enforce "unary must have one message." If the application closes with OK but no messages, this should become an error of some sort (sent to the client). This would ideally be done in ServerCallImpl.
From grpc/grpc#8010 (comment):
Is the server calling
StreamObserver.onCompleted()(orServerCall.close(Status.OK,...)) without first sending a message?[...] no problem. It made us realize we weren't verifying that on server-side.
From grpc/grpc-java#2243:
It appears that a server can respond OK to a unary RPC without any detection on the server-side. It'd be nice to throw an exception if close(OK) is called without first sending a message for a UNARY response, although that may be against our API stability. At the very least, we should convert the OK into a Status saying the server is broken.
There was a problem hiding this comment.
For unary RPC, I have not changed the final behavior and in this case there will be DATA_LOSS error code. See test at line 313.
In general, while there is no unambiguous interpretation yet of this case with streams, it is better to have an implementation compatible with other libraries so that it is easier for developers to migrate from one implementation to another.
There was a problem hiding this comment.
Ahhh, my mistake. This makes sense for serverStreaming.
|
Released in v2.8.0. |
Fixes #330