Fix OkHttp/Retrofit interceptor mess and improve web sockets

pom.xml

@@ -9,7 +9,7 @@
   <version>1.0.0</version>
 
   <properties>
-    <com.squareup.retrofit2.version>2.3.0</com.squareup.retrofit2.version>
+    <com.squareup.retrofit2.version>2.4.0</com.squareup.retrofit2.version>
   </properties>
 
   <dependencies>

src/main/java/com/binance/api/client/BinanceApiClientFactory.java

@@ -4,6 +4,8 @@
 import com.binance.api.client.impl.BinanceApiRestClientImpl;
 import com.binance.api.client.impl.BinanceApiWebSocketClientImpl;
 
+import static com.binance.api.client.impl.BinanceApiServiceGenerator.getSharedClient;
+
 /**
  * A factory for creating BinanceApi client objects.
  */
@@ -68,6 +70,6 @@ public BinanceApiRestClient newRestClient() {
    * Creates a new web socket client used for handling data streams.
    */
   public BinanceApiWebSocketClient newWebSocketClient() {
-    return new BinanceApiWebSocketClientImpl();
+    return new BinanceApiWebSocketClientImpl(getSharedClient());
   }
 }

src/main/java/com/binance/api/client/BinanceApiWebSocketClient.java

@@ -60,5 +60,9 @@ public interface BinanceApiWebSocketClient extends Closeable {
      */
     Closeable onAllMarketTickersEvent(BinanceApiCallback<List<AllMarketTickersEvent>> callback);
 
+    /**
+     * @deprecated This method is no longer functional. Please use the returned {@link Closeable} from any of the other methods to close the web socket.
+     */
+    @Deprecated
     void close();
 }

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

@@ -5,42 +5,53 @@
 import com.binance.api.client.exception.BinanceApiException;
 import com.binance.api.client.security.AuthenticationInterceptor;
 import okhttp3.OkHttpClient;
+import okhttp3.RequestBody;
+import okhttp3.ResponseBody;
 import org.apache.commons.lang3.StringUtils;
 import retrofit2.Call;
+import retrofit2.Converter;
 import retrofit2.Response;
 import retrofit2.Retrofit;
 import retrofit2.converter.jackson.JacksonConverterFactory;
 
 import java.io.IOException;
 import java.lang.annotation.Annotation;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Generates a Binance API implementation based on @see {@link BinanceApiService}.
  */
 public class BinanceApiServiceGenerator {
+    private static final OkHttpClient sharedClient = new OkHttpClient.Builder()
+            .pingInterval(20, TimeUnit.SECONDS)
+            .build();
 
-    static OkHttpClient.Builder httpClient = new OkHttpClient.Builder();
+    private static final Converter.Factory converterFactory = JacksonConverterFactory.create();
 
-    private static Retrofit.Builder builder =
-        new Retrofit.Builder()
-            .baseUrl(BinanceApiConstants.API_BASE_URL)
-            .addConverterFactory(JacksonConverterFactory.create());
-
-    private static Retrofit retrofit = builder.build();
+    @SuppressWarnings("unchecked")
+    private static final Converter<ResponseBody, BinanceApiError> errorBodyConverter =
+            (Converter<ResponseBody, BinanceApiError>)converterFactory.responseBodyConverter(
+                    BinanceApiError.class, new Annotation[0], null);
 
     public static <S> S createService(Class<S> serviceClass) {
         return createService(serviceClass, null, null);
     }
 
     public static <S> S createService(Class<S> serviceClass, String apiKey, String secret) {
-        if (!StringUtils.isEmpty(apiKey) && !StringUtils.isEmpty(secret)) {
+        Retrofit.Builder retrofitBuilder = new Retrofit.Builder()
+                .baseUrl(BinanceApiConstants.API_BASE_URL)
+                .addConverterFactory(converterFactory);
+
+        if (StringUtils.isEmpty(apiKey) || StringUtils.isEmpty(secret)) {
+            retrofitBuilder.client(sharedClient);
+        } else {
+            // `adaptedClient` will use its own interceptor, but share thread pool etc with the 'parent' client
             AuthenticationInterceptor interceptor = new AuthenticationInterceptor(apiKey, secret);
-            if (!httpClient.interceptors().contains(interceptor)) {
-                httpClient.addInterceptor(interceptor);
-                builder.client(httpClient.build());
-                retrofit = builder.build();
-            }
+            OkHttpClient adaptedClient = sharedClient.newBuilder().addInterceptor(interceptor).build();
+            retrofitBuilder.client(adaptedClient);
         }
+
+        Retrofit retrofit = retrofitBuilder.build();
         return retrofit.create(serviceClass);
     }
 
@@ -65,7 +76,13 @@ public static <T> T executeSync(Call<T> call) {
      * Extracts and converts the response error body into an object.
      */
     public static BinanceApiError getBinanceApiError(Response<?> response) throws IOException, BinanceApiException {
-        return (BinanceApiError)retrofit.responseBodyConverter(BinanceApiError.class, new Annotation[0])
-            .convert(response.errorBody());
+        return errorBodyConverter.convert(response.errorBody());
+    }
+
+    /**
+     * Returns the shared OkHttpClient instance.
+     */
+    public static OkHttpClient getSharedClient() {
+        return sharedClient;
     }
 }

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

@@ -9,7 +9,7 @@
 import com.binance.api.client.domain.event.DepthEvent;
 import com.binance.api.client.domain.event.UserDataUpdateEvent;
 import com.binance.api.client.domain.market.CandlestickInterval;
-import okhttp3.Dispatcher;
+
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.WebSocket;
@@ -22,12 +22,10 @@
  */
 public class BinanceApiWebSocketClientImpl implements BinanceApiWebSocketClient, Closeable {
 
-    private OkHttpClient client;
+    private final OkHttpClient client;
 
-    public BinanceApiWebSocketClientImpl() {
-        Dispatcher d = new Dispatcher();
-        d.setMaxRequestsPerHost(100);
-        this.client = new OkHttpClient.Builder().dispatcher(d).build();
+    public BinanceApiWebSocketClientImpl(OkHttpClient client) {
+        this.client = client;
     }
 
     public Closeable onDepthEvent(String symbol, BinanceApiCallback<DepthEvent> callback) {
@@ -55,10 +53,11 @@ public Closeable onAllMarketTickersEvent(BinanceApiCallback<List<AllMarketTicker
         return createNewWebSocket(channel, new BinanceApiWebSocketListener<List<AllMarketTickersEvent>>(callback));
     }
 
+    /**
+     * @deprecated This method is no longer functional. Please use the returned {@link Closeable} from any of the other methods to close the web socket.
+     */
     @Override
-    public void close() {
-        client.dispatcher().executorService().shutdown();
-    }
+    public void close() { }
 
     private Closeable createNewWebSocket(String channel, BinanceApiWebSocketListener<?> listener) {
         String streamingUrl = String.format("%s/%s", BinanceApiConstants.WS_API_BASE_URL, channel);