cjihrig/grpc-server-js

Ping Frame / Keep alive

SProst opened this issue · 6 comments

SProst commented

The default timeout for the http2 session is 2 minutes. Should a setInterval for pinging be established in the call class based on the defaults specified here for the server? https://github.com/grpc/grpc/blob/56006f0c532fd98a2b327852c763778279616e01/doc/keepalive.md

Per the guidance linked above, if you configure the client to send ping frames the server should receive the ping frame and respond. In addition, it might be useful to implement this: GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS.

cjihrig commented

Yes, I think the server should implement keepalive pings. I think the best place to start is with support for:

  • grpc.keepalive_time_ms, with a default value of 7200000 (2 hours).
  • grpc.keepalive_timeout_ms, with a default value of 20000 (20 seconds).
  • 🎉1
SProst commented

@cjihrig There is one other number to be aware of potentially. I originally stumbled across this because I have a C# client and the channel setting GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS defaults on the client to 5 minutes, which is 3 minutes longer than the default http2 server timeout. A long running process might not return a value in 2 minutes, which would cause the server to send a GOAWAY. Of course the client won't even attempt to check if the server is still up until 5 minutes have passed. Would it make sense to set the default timeout for the http2 server at some multiple of the GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS ?

cjihrig commented

Would it make sense to set the default timeout for the http2 server at some multiple of the GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS ?

I think the answer might be to disable session timeouts if possible. If that's not possible, maybe it could be set to the highest possible value. By default, gRPC requests are supposed to have an infinite deadline, so Node probably shouldn't be closing the connection on its own at any point.

  • 👍1
SProst commented

@cjihrig I was able to disable the timeout by setting it to 0, and have the callback be a noop.

cjihrig commented

The first step of this, disabling the timeout, is now implemented in 1eaf0cf. Additionally, this behavior has been disabled in Node's master branch in nodejs/node#27558.

  • 👍1
cjihrig commented

This has been published in version 0.1.4. Again, thanks for the issues and feature requests.

  • 🎉1