Skip to content
This repository has been archived by the owner on Jun 8, 2020. It is now read-only.

Generic WebsocketHealthWatcher thread #94

Closed
wants to merge 36 commits into from

Conversation

firegnome
Copy link
Contributor

@firegnome firegnome commented Feb 6, 2018

Implementation for #80

Since there is no generic way to find out if the websocket is still healthy, i wrote a method (isWebsocketReconnectRequired) that indicates if the socket needs to be reconnected. I implemented this for poloniex2 and its working for me.

@nodarret I tried my best to integrate your changes from #89 but I would really appreciate it If you could look at my changes.

As @hlbkin mentioned, there is a problem with the orderbooks:

The only problem I see is that when you reconnect, orderBooks become stale i.e. need to be reconnected and submitted full cycle (GDAX full channel for example or binance).

This problem isn't solved yet but i propose to create a new ticket, since this isn't really caused by this pull request.

dozd and others added 30 commits November 23, 2017 16:17
[maven-release-plugin] copy for tag xchange-stream-parent-4.3.0
# Conflicts:
#	service-netty/src/main/java/info/bitrich/xchangestream/service/netty/NettyStreamingService.java
#	xchange-poloniex2/src/main/java/info/bitrich/xchangestream/poloniex2/PoloniexStreamingService.java
…rmal timeout and set a timeout for reconnect to prevent infinite lock
# Conflicts:
#	service-netty/src/main/java/info/bitrich/xchangestream/service/netty/NettyStreamingService.java
# Conflicts:
#	service-netty/src/main/java/info/bitrich/xchangestream/service/netty/NettyStreamingService.java
@hlbkin
Copy link

hlbkin commented Feb 6, 2018

For Bitfinex, they have explicit messages that state when you need to disconnect (https://bitfinex.readme.io/v2/docs/ws-general). is your pull request == workaround for that? or you have seen disconnects from Bitfinex WITHOUT this explicit message?

@firegnome
Copy link
Contributor Author

firegnome commented Feb 6, 2018

@hlbkin There are disconnects WITHOUT this explicit message, for example if you connect to a VPN network it will drop the connection without message.

@dozd
Copy link
Member

dozd commented Feb 7, 2018

Hello @firegnome , thank you for the PR. I'll go through it after my return from the vacation. It seems a little bit complicated on the first look so I'd like to understand it more before merging.

The main question is about reusability of the code for other exchanges. Is this the common pattern that will be useful elsewhere?

Second concern is about using new Thread() with Thread.Sleep in "rx world". Some sort of rx timer or interval would be preferred to keep code consistent.

But both are not crucial.

@firegnome
Copy link
Contributor Author

Hi @dozd
To your first question: It is designed to be reusable for other exchanges. We had a discussion in #80 and other people confirmed, that this is needed on other exchanges too.

Thank you for your hint with rx timer i will have a look at this and update the PR.

@nodarret
Copy link
Contributor

When I have free time I wil check code. For now im trying speed up makret data #101 or write my own solutnion ;)

@hleb-albau
Copy link

hleb-albau commented Mar 13, 2018

General case -> start any stream, turn off Internet for 10min, turn on Internet. Application will be zoombied. Very interested in solution.

@badgerwithagun
Copy link
Collaborator

Necroing this:

I have found that with #191 applied, connections now auto-disconnect and reconnect pretty well when something goes wrong, and do so without interrupting client code.

#227 is written in such a way that the Binance orderbook recovers cleanly and I've not seen any issues with the GDAX orderbook.

I still don't trust anything, so I also run a separate heartbeat in my production app which makes sure it continuously receives trades from the Binance socket. If it doesn't receive any in a few minutes, it notifies me. In practice, this has never actually fired in production since I implemented it.

This feature looks sensible, but there's a chance its reason for being no longer exists.

@Flemingjp
Copy link
Collaborator

Is a heartbeat a given endpoint? It seems like an over engineered ping/pong transaction - which has now been implemented properly

@badgerwithagun
Copy link
Collaborator

Well, the ping/pong now implemented is more so that the exchanges don't disconnect from their end. It doesn't tell us if a connection is up but has "gone quiet" - we're not sending pings, just responding to them.

I've never seen it where a socket is open but the exchange has stopped sending data. I HAVE seen it where we have reconnected but failed to send resubscription messages and the connection just starts cycling up and down.

I suspect this predates the relatively reliable auto reconnection logic, but it's hard to say.

@badgerwithagun
Copy link
Collaborator

Heads up: I'm going to close this PR this week unless anyone feels strongly otherwise.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants