Error handling and shutdown improvements #49

big-andy-coates · 2018-01-25T11:27:18Z

This PR should hopefully improve the error handling in the client and allow users to shutdown the client and/or a active web socket.

Optional onFailure added to BinanceApiCallback to allow user to opt-in to receiving failure info, (Note: the interfacer remains a functional one for simple use-cases, it just now also supports error handling for when you care about such things).
BinanceApiWebSocketClient enhanced to return Closeables from calls creating web sockets, so that said web sockets can be later closed, if needed.
BinanceApiWebSocketClient enhanced to be Closeable itself, closing the internal OKHttp dispatcher.
BinanceApiWebSocketListener no longer throws an exception from onFailure, as this was just being thrown up the stack to the Thread.uncaughtExceptionHandler.

With these changes I can now:

detect failures on the web socket and choose to do something about them.
shutdown the BinanceApiWebSocketClient, and potentially recreate later, without any resource leaks or references being held to my callback objects.

… and/or a active web socket. - Optional `onFailure` added to `BinanceApiCallback` to allow user to opt-in to receiving failure info. - `BinanceApiWebSocketClient` enhanced to return `Closeable`s from calls creating web sockets, so that said web sockets can be later closed, if needed. - `BinanceApiWebSocketClient` enhanced to be `Closeable` itself, closing the internal OKHttp dispatcher. - `BinanceApiWebSocketListener` no longer throws an exception from `onFailure`, as this was just being thrown up the stack to the `Thread.uncaughtExceptionHandler`. With these changes I can now: - detect failures on the web socket and choose to do something about them. - shutdown the `BinanceApiWebSocketClient`, and potentially recreate later, without any resource leaks or references being held to my callback objects.

xezon

Very nice.

xezon · 2018-01-27T16:54:38Z

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

-    client.newWebSocket(request, listener);
+    final WebSocket webSocket = client.newWebSocket(request, listener);
+    return () -> {
+      final int code = 1000;


Any particular reason for code value 1000 ?

1000 is the status code for a normal ws shutdown, basically telling the server 'Thanks, I'm done'.

xezon · 2018-01-27T16:56:17Z

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

  @Override
  public void onFailure(WebSocket webSocket, Throwable t, Response response) {
-    throw new BinanceApiException(t);
+    if (!closing) {


I wonder if there should still be some sort of notification to inform about the closure, because a regular API user might not see the closure of the socket, leaving him wondering why his application no longer works after a time.

Unfortunately, without this, when the user calls close on the web socket, they user would receive a EOFException on this onFailure callback. This is not intuitive IMHO.

The flag stops the EOFException that occurs in response to the user's close request.

Yea but if the user does not close it and does not check onFailure, then the socket closes unnoticed.

xezon · 2018-01-27T17:01:45Z

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

    String streamingUrl = String.format("%s/%s", BinanceApiConstants.WS_API_BASE_URL, channel);
    Request request = new Request.Builder().url(streamingUrl).build();
-    client.newWebSocket(request, listener);
+    final WebSocket webSocket = client.newWebSocket(request, listener);
+    return () -> {


When is this function object called? Or how to use it?

This is used by the clients of the library. They can call close on the returned Closeable to close the websocket.

big-andy-coates · 2018-01-31T19:33:23Z

Hey @xezon - is there anything stopping this being merged?

xezon · 2018-02-01T13:24:21Z

I am not the owner of this repository, so I certainly don't block anything.

mcourteaux · 2018-02-01T13:37:53Z

I'll join this conversation, as I'm also stumbling upon the EOFExceptions. I think that if you want to close the client, that you need to close the Websockets, as they live on their own thread. Simply doing:

public void close() {
   client.dispatcher().executorService().shutdown();
}

won't exactly close the streams. This will cause the ExecutorService (which is a standard Java API class) to stop accepting new tasks (Callable).

mcourteaux · 2018-02-01T15:18:34Z

We will be using your PR coming days. If any trouble arises, I'll get back here.

big-andy-coates · 2018-02-01T22:53:59Z

Hi @mcourteaux, I'd agree. The websocket is closed when the following line is invoked:

webSocket.close(code, null);

Which is invoked here.

The call to shutdown the executor service, which you call out above, was not added as part of this PR, but which is part of the documented way of shutting down the OkHttp client.

big-andy-coates · 2018-02-01T22:55:31Z

I am not the owner of this repository, so I certainly don't block anything.

Ah, fair enough @xezon - appreciate the review!

big-andy-coates · 2018-02-01T22:57:19Z

Hey @joaopsilva - any chance of a review please?

lange88 · 2018-02-03T21:15:03Z

Hi @datalorax I ran into EOF exceptions as well and I am using your solution to restart the websockets after they are closed.

I am running into another problem though, after ~16 hours of streaming the web sockets are no longer receiving any data (no exceptions are thrown, the data just stops). I would like to be able to close the current web socket and start a new one. Any idea how I would go about doing that? I have tried dereferencing the BinanceApiWebSocketClient object, calling the GC and creating a new object, but that does not work. Maybe it would be nice to add manual shutdown functionality as well?

Edit: I think I managed already, the BinanceApiWebSocketClient interface functions return Closeables which you can use to close the websockets.

big-andy-coates · 2018-02-04T14:56:26Z

Edit: I think I managed already, the BinanceApiWebSocketClient interface functions return Closeables which you can use to close the websockets.

Yep, that's correct. I've added Javadocs to BinanceApiWebSocketClient to make this more clear.

-Added Java docs to make it clear you can close the websocket by calling close on the returned `Closeable`

joaopsilva · 2018-02-04T23:46:17Z

@datalorax @xezon I very much appreciate your efforts here, it looks good, but I need some more time to review these changes properly, thank you for understanding.

big-andy-coates · 2018-02-05T11:38:20Z

@jaggedsoft sure, no problem. Anything I can do to help, (better docs, clarifications, etc), just ask.

big-andy-coates · 2018-02-18T18:34:17Z

@jaggedsoft any idea when/if this can be merged?

joaopsilva · 2018-02-23T10:03:18Z

Thanks @big-andy-coates.

big-andy-coates · 2018-02-23T13:53:20Z

@joaopsilva thanks the review!

…dling_and_shutdown Error handling and shutdown improvements.

big-andy-coates changed the title ~~Improvements to error handling and the ability to shutdown the client…~~ Error handling and shutdown improvements Jan 25, 2018

This was referenced Jan 25, 2018

Expose WebSockets for graceful closing #47

Closed

Add callback null event on websocket failure #34

Closed

Exception in thread "OkHttp Dispatcher" com.binance.api.client.exception.Binance #24

Closed

xezon reviewed Jan 27, 2018

View reviewed changes

Merge branch 'master' into error_handling_and_shutdown

c3cd9e1

big-andy-coates mentioned this pull request Jan 29, 2018

added option to close individual websockets #57

Closed

joaopsilva mentioned this pull request Jan 29, 2018

BinanceApiCallback must have an onFailure() method #58

Closed

-Removed unused imports

a10dc0c

-Added Java docs to make it clear you can close the websocket by calling close on the returned `Closeable`

Andy Coates added 2 commits February 5, 2018 11:50

Add error handling and closure of websockets to readme

8664140

code review improvements

37ef9a0

gizmochief7 mentioned this pull request Feb 7, 2018

StackOverflowError with the AuthenticationInterceptor after several hours of swing trading #54

Closed

joaopsilva merged commit c64cee1 into binance-exchange:master Feb 23, 2018

big-andy-coates deleted the error_handling_and_shutdown branch February 23, 2018 13:53

nilswx pushed a commit to nilswx/binance-java-api that referenced this pull request Dec 14, 2018

Merge pull request binance-exchange#49 from big-andy-coates/error_han…

5546c7e

…dling_and_shutdown Error handling and shutdown improvements.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling and shutdown improvements #49

Error handling and shutdown improvements #49

big-andy-coates commented Jan 25, 2018 •

edited

Loading

xezon left a comment

xezon Jan 27, 2018

big-andy-coates Jan 29, 2018

xezon Jan 27, 2018

big-andy-coates Jan 29, 2018

xezon Feb 1, 2018

xezon Jan 27, 2018

big-andy-coates Jan 29, 2018

big-andy-coates commented Jan 31, 2018

xezon commented Feb 1, 2018

mcourteaux commented Feb 1, 2018

mcourteaux commented Feb 1, 2018

big-andy-coates commented Feb 1, 2018 •

edited

Loading

big-andy-coates commented Feb 1, 2018

big-andy-coates commented Feb 1, 2018

lange88 commented Feb 3, 2018 •

edited

Loading

big-andy-coates commented Feb 4, 2018 •

edited

Loading

joaopsilva commented Feb 4, 2018

big-andy-coates commented Feb 5, 2018

big-andy-coates commented Feb 18, 2018

joaopsilva commented Feb 23, 2018

big-andy-coates commented Feb 23, 2018

Error handling and shutdown improvements #49

Error handling and shutdown improvements #49

Conversation

big-andy-coates commented Jan 25, 2018 • edited Loading

xezon left a comment

Choose a reason for hiding this comment

xezon Jan 27, 2018

Choose a reason for hiding this comment

big-andy-coates Jan 29, 2018

Choose a reason for hiding this comment

xezon Jan 27, 2018

Choose a reason for hiding this comment

big-andy-coates Jan 29, 2018

Choose a reason for hiding this comment

xezon Feb 1, 2018

Choose a reason for hiding this comment

xezon Jan 27, 2018

Choose a reason for hiding this comment

big-andy-coates Jan 29, 2018

Choose a reason for hiding this comment

big-andy-coates commented Jan 31, 2018

xezon commented Feb 1, 2018

mcourteaux commented Feb 1, 2018

mcourteaux commented Feb 1, 2018

big-andy-coates commented Feb 1, 2018 • edited Loading

big-andy-coates commented Feb 1, 2018

big-andy-coates commented Feb 1, 2018

lange88 commented Feb 3, 2018 • edited Loading

big-andy-coates commented Feb 4, 2018 • edited Loading

joaopsilva commented Feb 4, 2018

big-andy-coates commented Feb 5, 2018

big-andy-coates commented Feb 18, 2018

joaopsilva commented Feb 23, 2018

big-andy-coates commented Feb 23, 2018

big-andy-coates commented Jan 25, 2018 •

edited

Loading

big-andy-coates commented Feb 1, 2018 •

edited

Loading

lange88 commented Feb 3, 2018 •

edited

Loading

big-andy-coates commented Feb 4, 2018 •

edited

Loading