Reconnecting websocket onclose #40
Conversation
| } | ||
| } | ||
|
|
||
| let closedManually = false |
There was a problem hiding this comment.
Not a fan of this hacky flag. Shouldn't we check the code of the event emitted on close and infer if it has been triggered manually or not? Referring to section 7.4.1 of RFC6455.
There was a problem hiding this comment.
Neither am I, and initially I was implementing this with using a code but after seeing the amount of possible values for the code we need to check + the uncertainty of what codes we'll get working with binance's unstable servers + the general instability of websockets api + the difficulty of testing this + lack of time in my hands I see this simple approach as much more efficient, simpler, and easier to understand than trying to be spec compliant
src/websocket.js
Outdated
| } | ||
|
|
||
| let closedManually = false | ||
| let w = newSocket(path, cb, onClose) |
There was a problem hiding this comment.
w should be defined before onClose in this scope
There was a problem hiding this comment.
are you saying it should be defined or declared before onClose? Because it cannot be defined before onClose as we need to pass onClose as one of the parameters to newSocket()
src/websocket.js
Outdated
|
|
||
|
|
||
| const createSocket = (path, cb) => { | ||
| const onClose = ()=> { |
There was a problem hiding this comment.
Might want to run prettier too
| }) | ||
|
|
||
| return () => cache.forEach(w => w.close()) | ||
| return () => cache.forEach(close => close()) |
There was a problem hiding this comment.
As per #36, I'm thinking it might be interesting to return an object exposing this clean function + the websockets array for advanced users, thoughts?
There was a problem hiding this comment.
Sure that'll be useful but as I mentioned before the problem with this approach is that it's going to break any apps using the existing api with the close callbacks as this is not backwards compatible. Plus, if the purpose of returning the websocket object back to the caller is so that the caller can handle errors and reconnect by himself then it won't be necessary if reconnection is handled internally by this lib (which I think it should)
There was a problem hiding this comment.
We can do a major release, that's not a big deal
|
|
||
| const newSocket = (path, cb, onClose) => { | ||
| const w = new WebSocket(`${BASE}/${path}`) | ||
| const onDisconnect = () => setTimeout(onClose, RECONNECT_DELAY) |
There was a problem hiding this comment.
Any reason not to reconnect instantly?
There was a problem hiding this comment.
Because binance's servers have proven to be awfully unstable and at times down for hours. If we do not apply a delay we can potentially cause a huge overload if their servers are down and we are constantly trying to reconnect. If their servers are down for 1 minute and are immediatly rejecting the connection we can know that with the 1s delay we will be instantiating ~50 websocket instances/attempted connections while if we don't apply a delay we can be looking (and considering it takes 10ms to create a websocket and emit a connection) we will be looking at ~6000 attempted connections in that 1min.
|
What do you think of sockette, a ws wrapper automatically handling reconnection? I'm thinking of using this instead |
+1 for reusing existing libraries. Here is another challenger: reconnecting-websocket |
|
Thanks for your help! Very sorry about the delay, and I'll to close in favor of #49 which is tested |
#38 pull request which I created only handles reconnecting on
errorevents but after using that branch in production env I quickly realized that there are many cases in which the connection gets disconnected for whatever reason and doesn't emit anerrorevent but rather just acloseevent. This branch is modified to handle these close events and also distinguishes between a user-invoked close event and a non-user-invoked closed event.