improbable-eng / grpc-web

gRPC Web implementation for Golang and TypeScript
Apache License 2.0
4.37k stars 436 forks source link

grpcwebproxy: websocket is closed before all frames are processed causing grpc-status is not identified through trailers. #663

Open dbq817 opened 4 years ago

dbq817 commented 4 years ago

Versions of relevant software used

grpcweb 0.12.0 grpcwebproxy

What happened

on iOS platform, client issues unary calls to backend server through grpcwebproxy, some of them may fail due to grpc-status is missing, and this is reproducible during a period of observation.

What you expected to happen

according to the wire protocol, trailer headers are always returned, and grpc-status is critical for the client library to identify if the call succeeded or not.

Full logs to relevant components

``` 2020-02-21 08:41:34.226 [tid:com.facebook.react.JavaScript] grpc.onEnd 2020-02-21 08:41:34.226 [tid:com.facebook.react.JavaScript] 'grpc.headers only response ', null, [] 2020-02-21 08:41:34.226 [tid:com.facebook.react.JavaScript] 'rawOnEnd', 2, 'Response closed without grpc-status (Headers only)', { headersMap: { 'content-type': [ 'application/grpc+proto' ], 'grpc-accept-encoding': [ 'gzip' ], trailer: [ 'Grpc-Status', 'Grpc-Message', 'Grpc-Status-Details-Bin' ] } } 2020-02-21 08:41:34.228 [info][tid:com.facebook.react.JavaScript] 'time:', Fri Feb 21 2020 08:41:34 GMT+0800 (CST) 2020-02-21 08:41:34.230 [info][tid:com.facebook.react.JavaScript] { status: 2, statusMessage: 'Response closed without grpc-status (Headers only)', headers: { headersMap: { 'content-type': [ 'application/grpc+proto' ], 'grpc-accept-encoding': [ 'gzip' ], trailer: [ 'Grpc-Status', 'Grpc-Message', 'Grpc-Status-Details-Bin' ] } }, message: { wrappers_: {}, messageId_: undefined, arrayIndexOffset_: -1, array: [ , 'OKAY' ], pivot_: 1.7976931348623157e+308, convertedPrimitiveFields_: {} }, trailers: { headersMap: { 'content-type': [ 'application/grpc+proto' ], 'grpc-accept-encoding': [ 'gzip' ], trailer: [ 'Grpc-Status', 'Grpc-Message', 'Grpc-Status-Details-Bin' ] } } } ```

-->

Anything else we need to know

jump to RFC 6455, it is said that

on some platforms, if a socket is closed with data in the receive queue, a RST packet is sent, which will then cause recv() to fail for the party that received the RST, even if there was data waiting to be read

jump again to the websocket library which is used by grpcwebproxy, https://github.com/gorilla/websocket, I found that tcp connection is closed directly without sending a close message

// Close closes the underlying network connection without sending or waiting
// for a close message.
func (c *Conn) Close() error {
    return c.conn.Close()
}

so now I think the root cause lies here. my solution to this is let the client close the socket after a close message is received, and the only thing the server does is merely send close message after everything is done.

johanbrandhorst commented 4 years ago

Hi, thanks for your detailed bug report. We've long noted that the websocket transport is not suitable for production use, and it is not bound by the grpc-web spec, it's basically it's own unique implementation. I would recommend you switch away from the websocket transport, if possible.

Regardless, this is of course something we'd like to fix, but I can't promise we'll have the spare cycles to do so. Would you be interested in contributing a test and a fix?

dbq817 commented 4 years ago

yes, for unary calls we switched to xhr based transport, but for stream calls we have no other options for now :(. I'd like to take sometime to get familiar with golang and then get back trying to fix this issue.

johanbrandhorst commented 4 years ago

The Fetch and XHR transport both support server-side streaming. Client-side streaming is unfortunately impossible though. Might help with some of your use cases?

dbq817 commented 4 years ago

we tried default transport at beginning, but it was found that it is not stable enough, say it could get disconnected periodically both on pc and mobile platforms, so we switched to use websocket transport.

our use case is quite normal, which is to build an in-app im feature, and unary calls are for sending/synchronizing messages, and stream call is for listening to realtime messages.

@johanbrandhorst

johanbrandhorst commented 4 years ago

I think your best bet is to switch back to the normal transport and implement some sort of manual keepalive and reconnection. Until https://github.com/grpc/grpc-web/issues/24 is resolved, we're unlikely to have official websocket support.