Fix OkHttp/Retrofit interceptor mess and improve web sockets

[nilswx]

Hey! I discovered more problems with the current state of the library. OkHttp and Retrofit were used incorrectly, so I decided to improve it. With a few quick modifications I was able to address some issues that have been raised before. It's still no immutable Scala heaven, but it works! 🙃

Background

It's recommended that there is only one OkHttpClient per process, which will manage a Dispatcher and OkHttpConnectionPool to coordinate all (a)synchronous requests, including the handshake requests for web socket negotiation.

AuthenticationInterceptor

I found out that there was some truly wacky shit going on in BinanceApiServiceGenerator. On every 'service generation' for an authenticated (a)synchronous REST API client there was this weird dance involving the modification of the shared OkHttpClient by adding a newAuthenticationInterceptor. This updated client was then plugged into a (shared/static) instance of Retrofit that kept getting modified behind the scenes before it would be returned to callers. This was asking for trouble, especially in the case of 'multi tenancy'. (#99, #123)

BinanceApiServiceGenerator now properly uses the existingClient.newBuilder().interceptor(someInterceptor).build() pattern to make a shallow clone of the shared OkHttpClient and have this clone use a specific AuthenticationInterceptor, while still using the connection/thread pool and other resources of the process-wide shared client. This adapted client is then used to create a new Retrofit instance, which then materializes the service.

Web sockets

During testing I discovered that the web sockets weren't properly detecting disconnections (#96). It could take up to a few minutes before disconnections of the web socket (or internet) would be detected and propagated as exceptions. In my use case this was unacceptable.

I attempted to improve this by setting a very small pingInterval on the OkHttpClient, but this had no effect. It turns out that there was a problem with the version of OkHttp used by Retrofit 2.3. I learned that upgrading to a newer version of OkHttpClient would fix this, and it did! I upgraded Retrofit to 2.4 (which uses the newer version of OkHttp) and hardcoded (!) a sensible pingInterval of 20 seconds to detect disconnections, which is working perfectly.

Additionally I noticed that every BinanceApiWebSocketClientImpl was spawning its own OkHttpClient, with its own Dispatcher and thread pool. This is not suited for multi tenancy and leads to an unnecessary amount of (idle) threads. I changed this so that BinanceApiWebSocketClientImpl now uses the same process-wide shared OkHttpClient as the rest of the application.

Please review the changes attached and let me know what you think! 🌽

[joaopsilva]

Thank you for this @nilswxa , looks promising, I will review the changes this week.

[mironbalcerzak]

@joaopsilva were those changes merged or was this PR rejected? I see none of the proposed in the code base.

[joaopsilva]

Okay, lgtm, I'll change later the ping timeout, as I'm not so convinced that it's a good idea to force every client to use that value.

[waeron]

Hi @nilswxa ,

I have the latest library version with your updates, but it does not helps to resolve issue with unstable connection and random disconnections (15 - 60 minutes) from web sockets using the onAccountUpdate method. Even after updating related libraries in POM and increasing/decreasing of pingInterval value. Do you have any ideas how to resolve random disconnections from client side?

Thanks