send PONG message

[jaime-ez]

Hi, I'm working on sending a PONG message in response to PING messages from the client, specifically on the message-procesor, where there is a comment that // Each connection endpoint is responsible for dealing with ping connections, but I think it's easier to just handle it there.

Any comments?

[yasserf]

Hello!

The reason it’s in different connection endpoints is because different libraries respond differently to heartbeats. uWS for example has an idle timeout that kills the connection, ws does nothing and requires that logic to be implemented, and mqtt uses its own ping / pong mechanism.

In V3 I believe the server sent out pings and the clients responded to pongs, which is the text protocol.

Also in uWS you don’t need to recieve a ping for an idle connection to be reset, it’s any message really (and vice versa in the client and a pong). So sending pings and responding is only needed if the connection is quiet for the entire duration of the heartbeat timeout

[jaime-ez]

Ok, I came around this while looking at the client's socket factory code where I realized there was a ping message being sent every heartBeatInterval, and a getTimeSinceLastMessage function that always returns 0. Thus making the checkHeartBeat function here redundant. Thats why I thougt sending a PONG message in response to the PING will re-enable the clients code.

So if the ping/pong logic is not necessary, and only the ws server requires it, maybe the client's checkHeartBeat and getTimeSinceLastMessage functions could be removed?

I agree it is not an issue, but l think its best not to have unnecesary timers floating around...unless there is something else I am missing of course

[yasserf]

Thanks for delving deeper

https://github.com/deepstreamIO/deepstream.io-client-js/blob/e2c857a20628de1d1b5941a76b936cadb378eee0/src/connection/socket-factory.ts#L39

That line should be enabled, otherwise the client will never disconnect due to a hearbeat timeout (which actually explains some of the issues people are facing with connections not dying with terrible mobile signals)

The tests override the actual function getTimeSinceLastMessage which is why they work

So to summarise:

• Ping pong logic is necessary
• The client sends pings (https://github.com/deepstreamIO/deepstream.io-client-js/blob/e2c857a20628de1d1b5941a76b936cadb378eee0/src/connection/socket-factory.ts#L67)
• The server should respond with pongs (https://github.com/deepstreamIO/deepstream.io/blob/master/src/connection-endpoint/base/connection-endpoint.ts#L209, https://github.com/deepstreamIO/deepstream.io/blob/master/src/connection-endpoint/base/connection-endpoint.ts#L183).
• The client and server checks that any message was received within the heartbeat interval to ensure connection is open. So the ping pongs just guarantee we got that message.

So it appears the binary/json protocol doesn't do pings, the text protocol (v3) does (https://github.com/deepstreamIO/deepstream.io/blob/f5dd849b7509bdbe6aafebf373dad78fb0e313c9/src/connection-endpoint/websocket/text/connection-endpoint.ts#L15). So I'm guessing as long as the V3 client doesn't send as a PING (which it doesn't since it responds with pongs which are ignored by the server https://github.com/deepstreamIO/deepstream.io/blob/46ed20885951a27eb87f79080da098320c6fcfe0/src/connection-endpoint/websocket/text/text-protocol/message-parser.ts#L225) than we are good.

Generally many thanks for you raising this issue, definitely unearths some issues.

[jaime-ez]

Ok, so you agree that the ping pong logic should be enabled. Thus enabling here

https://github.com/deepstreamIO/deepstream.io-client-js/blob/e2c857a20628de1d1b5941a76b936cadb378eee0/src/connection/socket-factory.ts#L39

I got a bit confused with this:

• The server should respond with pongs (https://github.com/deepstreamIO/deepstream.io/blob/master/src/connection-endpoint/base/connection-endpoint.ts#L209, https://github.com/deepstreamIO/deepstream.io/blob/master/src/connection-endpoint/base/connection-endpoint.ts#L183).

Here there are no actual pongs being sent, and those functions arent called upon a PING message sent by the client.

) than we are good.

by that you mean that we are good to ignore the comment on the message procesor and implement the PONG response?

I'll go ahead and make a pull request to clarify, sorry if I misunderstood what you meant

[yasserf]

Ok, so you agree that the ping pong logic should be enabled.

Yes

The server should respond with pongs

When the client sends the server a ping, it should respond with a pong (as you mentioned yes).

Generally it breaks protocol with V3 but since the client doesn't send pings should be fine 👍

[yasserf]

Awesome, thanks for finding and fixing this!

I'll close this once we merge in https://github.com/deepstreamIO/deepstream.io-client-js/pull/525

[jaime-ez]

Great!

[yasserf]

Implemented in 5.1. Thanks! 😁