0.7.X changes server keep alive semantics in client

[NeoPhi]

The SubscriptionClient document currently includes: timeout?: number : how long the client should wait in ms for a subscription to be started (default 5000 ms). However this is not how the parameter is being used. Instead this timeout is being used in the GQL_CONNECTION_KEEP_ALIVE handler to close the client to server connection if a server keep alive message is not received in that time. As implemented it feels like this is more of an idleTimeout. Additionally in previous versions the server sent keep alive were used to keep the underlying network connection open for long lived but infrequent subscriptions. Given our default server configuration this is also a parameter we will have to override in GraphiQL to ensure it doesn't close based on the default 5 second timeout.

[mistic]

good catch @NeoPhi!

[sorenbs]

At Graphcool we send the keep-alive message at 30 second intervals which in almost all cases is enough to keep the underlying connection open. As it stands the client with default configuration disconnects 5 seconds after the first keep-alive message arrives.

From the Apollo Slack subscriptions channel:

Is there any specific reason for decreasing the default keep-alive interval to 5 seconds? That is much lower than what I am used to from other systems. Either we have to change our server to send ka messages every 2 seconds (to account for network latency), the keep-alive logic in Apollo has to be much more lenient towards missed ka messages or the default interval in the client has to be increased to something more reasonable (say, 30 seconds)

[helfer]

@sorenbs 5s sounds too low for me too. But just to check: it's the server that's sending the keepalive message here and the client that disconnects if it doesn't receive it? Shouldn't the client instead be the one that says when it wants the connection to stay alive? Presumably the server shouldn't care if the client wants to disconnect (unless it wastes resources by reconnecting right after that)?

[sorenbs]

In our implementation the server is sending the ka message every 10 seconds no matter what the client does. This is simply to keep the connection open. The client is free to disconnect at any time, either by closing the connection or sending the disconnect message.

As I understand the current client implementation, it by default keeps the connection open forever. Then, when it receives the first ka message, it sets a timer for the specified timeout (default 5 sec) and sends the close connection message if a new ka message is not received before that time.

I'm not sure this client behaviour makes the most sense, but given this behaviour, I would like to see the default timeout increased.

[dotansimha]

I see. do you have another idea for implementing the timeout feature?

I think that the current implementation let the client decide how to behave with the server, and use the keep-alive as it needs.

I agree on the 5s issue, and I think it should be higher to avoid disconnections. Do you think that 10s is high enough or we should increase it?

@sorenbs

[sorenbs]

There is plenty prior art on this topic:

https://stackoverflow.com/questions/12815231/controlling-the-heartbeat-timeout-from-the-client-in-socket-io

http://docs.spring.io/spring/docs/current/spring-framework-reference/html/websocket.html#websocket-fallback-sockjs-heartbeat

Bumping the default timeout would resolve it for us, and I think 30 seconds is a good number.

[dotansimha]

@sorenbs we changed it to 30s and @Urigo will release a new version of the transport soon :)

[sorenbs]

Thanks all!

[Urigo]

released in 0.7.2