Fix ping interval for websocket clients

[eugenpodaru]

What does this PR do?

• Override the pingInterval of the OkHttpClient sharedClient for the BinanceApiWebSocketClients

Where should the reviewer start?

• src/main/java/com/binance/api/client/impl/BinanceApiWebSocketClientImpl.java
• src/main/java/com/binance/api/client/BinanceApiClientFactory.java

How should this be manually tested?

• Subscribing to a channel through a BinanceApiWebSocketClient

Any background context you want to provide?

According to the documentation:

The websocket server will send a ping frame every 3 minutes. If the websocket server does not receive a pong frame back from the connection within a 10 minute period, the connection will be disconnected. Unsolicited pong frames are allowed.

The current implementation uses the same pingInterval for all clients, and it has a value of 20s. This works for a while, but then starts failing with java.net.SocketTimeoutException: sent ping but didn't receive pong within 20000ms.

Setting the pingInterval to 3 minutes as per the documentation fixes the issue. I have been running the client with the fix in production for a few days already, and have not encountered the error, while before, I would get it an hour after the app restart, and every 20 seconds after the first failure.

[eugenpodaru]

I believe it fixes #282 #202

[mkurz]

Why not just change the interval here:

binance-java-api/src/main/java/com/binance/api/client/impl/BinanceApiServiceGenerator.java

Line 35 in 8d38e8e

.pingInterval(20, TimeUnit.SECONDS)

?

How did you come up with 3 minutes?

[eugenpodaru]

@mkurz The 3 minutes is from the Binance documentation.

The reason I am not changing it in the ServiceGenerator, is because the same client is used for the http connections, and they probably don't have the same pingInterval.

[mkurz]

The 3 minutes is from the Binance documentation.

The ping/pong from the documentation has nothing to do with the pingInterval you are changing here. The ping/pong documented (where the server is sending the ping and the client is answering with pong) is handled already by underlying okhttp, there is not even a setting to change any interval for that.

The pingInterval of the line I referred to or that you want to change now, is not even necessary (that's a ping the client will send and the server will answer with a pong). We could completely remove it and the client will still work (and the server ping/client pong will still be working and handled by underlying okhttp).
The pingInterval was just added so the client will faster recognice if the connection got aborted.

The reason I am not changing it in the ServiceGenerator, is because the same client is used for the http connections, and they probably don't have the same pingInterval.

I highly doubt that pingInterval is used by a http connection at all. You send a request, you get an answer, connection closed, AFAIK.

[eugenpodaru]

It says in the documentation that it's used for both HTTP/2 and websocket connections. It probably does not affect the http calls made from this Binance client, so I would just remove it from the shared http client, and set it only for the websockets client with an appropriate value.

You are right, pingInterval does not affect the server initiated ping-pongs, to which the documentation is referring. But setting it to 20 secs, just makes the client too eager to drop perfectly good connections (I was still receiving messages, when getting the exception). So setting it to 3 mins, to mirror what the server is doing seems reasonable.

[mkurz]

I totally agree. 20 seconds seems far to less.
BTW: You are referring to okhttp docs 4.x, however binance-java-api is using 3.x.

[eugenpodaru]

It says the same for okhttp 3.X

[kaushik1979]

Looks good to me. Can you merge it into master please?

[bullshit]

It seems to me that this is still an issue and @eugenpodaru isn't active anymore. Any plans to fix this?