Okhttp: Fail the WebSocket if a pong isn’t received within some reasonable period

[swankjesse]

When we do automatic pings it’d be nice to fail the connection if the peer isn’t ponging back. As-is a server that blackholes pings is not sufficient to consider the connection healthy.

See also https://github.com/square/okhttp/issues/3197

[ahulyk]

hi, any news on that?

[dryganets]

Hi @ahulyk,

Thank you for your PR!
I'm not from the OkHttp team, but we had the same problem, and we are using the older version of OkHttp to be able to control ping/pong logic for now.

Here are some my thoughts regarding your PR:

• There should be an option to set the timeout. For different use-cases, reasonable time could be different.
• It doesn't matter if you received the pong or not; it matters if you are getting any data through this socket. I.e., I could receive/send a file through the socket and as a result, I could miss the pong packet or process it only once the limit has passed. I don't want to have the socket to be disconnected in such cases.

[ivmos]

@dryganets Also interested in this PR but can you tell me which old okhttp version are you using as workaround? Thanks in advance.

[colymore]

Also failling for me.

[dryganets]

Pick any version below 3.5.0 as since this version WS is a part of OkHttp and have a slightly different API. It used to be a separate package before.

Version 3.4+ has ping method exposed, so it is possible to send pings and have a custom ping handling logic.

Also, if I remember correctly, 3.5+ doesn't require you to close the response body in onMessage callback while versions before will crash if the response body is not closed correctly.

[ivmos]

@dryganets thanks a lot

It doesn't matter if you received the pong or not; it matters if you are getting any data through this socket. I.e., I could receive/send a file through the socket and as a result, I could miss the pong packet or process it only once the limit has passed. I don't want to have the socket to be disconnected in such cases.

Agree but then also ping should only be performed if elapsed time since last received byte happened enough time ago (e.g more than configured ping interval time). As we understand ping is redundant if we're already getting traffic through the websocket..

That said I guess PR is a good start compared to current behaviour...

[ahulyk]

Pick any version below 3.5.0 as since this version WS is a part of OkHttp and have a slightly different API. It used to be a separate package before.

Using old version of OkHttp client is not quit good solution for me.

There should be an option to set the timeout. For different use-cases, reasonable time could be different.

Looks like we already have that in place - pingInterval. I am going to check that behavior.

As we understand ping is redundant if we're already getting traffic through the websocket.

I totally agree here. I am going to investigate that.

[dryganets]

Would be great to get some official opinion here.
I hope @swankjesse or somebody else from Square will chime in and set the official expectations for the PR.

[swankjesse]

I’d like to implement this functionality but I don’t think this PR is exactly the way I want to go.

[dryganets]

Just to clarify: Are you saying you are going to take care of it or you will come up with the better proposal once you will have a time and leave it for @ahulyk to fix accordingly?

[swankjesse]

I’d like to implement my own solution to this problem.

[colymore]

when you think it could be included?

[chax]

@swankjesse when can we expect your solution to be implemented or at least to merge @ahulyk's PR as an intermediate solution until you make your own implementation. Another solution would be that you again expose onPong() callback like you had in versions 3.0.x-3.4.x so we can implement our own handling of closing websockets that don't respond on pings with pongs.

[swankjesse]

Question for web socket users:

My preference is to fail the web socket if we don't receive a pong within the ping interval.

What's your ping interval? Does this new policy work for you?

[meoyawn]

@swankjesse our ping interval is 30 sec, but pong timeout is 10 sec. On timeout we fail the connection and try reconnecting. So if both ping interval and pong timeout are configured through the single value, this does not work for us

[chax]

@swankjesse this is exactly what we need.

[swankjesse]

Okay, I’ve done some thinking on this and have some observations. There are two interesting parameters:

• Maximum round trip time
• Maximum time to discover connectivity loss

With this PR we’re require our users to set the ping interval to max roundtrip time or greater.
In the best case we’ll detect a connectivity loss in the ping interval (connectivity lost immediately before we ping).
In the worst case we’ll detect a connectivity loss in 2 x the ping interval (connectivity lost immediately after we receive a pong).

With multiple pings in flight it’d be possible to improve the worst case. We can’t improve the best case. I’m okay with this.

[dryganets]

Hi @swankjesse,

Small correction here:
Any successfully received data packet should reset the ping timer not just a pong packet.

If there is an incoming traffic in the web socket it shouldn't be considered stale even in case there are no pong responses.
Otherwise, some long operations might cause false timeouts.

[JakeWharton]

Pongs can interleave and the spec tells you to reply as soon as possible.

On Tue, Feb 20, 2018, 5:57 PM Sergei Dryganets [e-mail]
wrote:

Hi @swankjesse https://github.com/swankjesse,

Small correction here:
Any successfully received data packet should reset the ping timer not just
a pong packet.

If there is an incoming traffic in the web socket it shouldn't be
considered stale even in case there are no pong responses.
Otherwise, some long operations might cause false timeouts.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/3227#issuecomment-367150608, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEWm9cl8HFA2dQKc23fv1_PUU3p7Sks5tW03MgaJpZM4MheB3
.

[dryganets]

yep, you are right - ping and pong are control frames so if server implements the standard correctly this should be good enough.

Some interesting details from the RFC:

If an endpoint receives a Ping frame and has not yet sent Pong frame(s) in response to previous Ping frame(s), the endpoint MAY elect to send a Pong frame for only the most recently processed Ping frame.

A Pong frame MAY be sent unsolicited.  This serves as a unidirectional heartbeat.  A response to an unsolicited Pong frame is not expected.

[swankjesse]

I’m hoping what we have here is sufficient. If it isn’t because pongs are head-of-line blocked behind messages, or because pongs are being sent unsolicited, then we can fix in follow-up.

[yschimke]

The unsolicited pong frame thing should be handled, timeouts will stop working if a server is using pongs for keepalives.

[Harlber]

It looks quite similar to my issue #3722 @yschimke

[vivekpanchal]

Hey, I am getting the same Error any Fix for this ??