Full support for websocket reconnection/resubscription #3167
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@cgewecke Sorry for commenting on this draft PR. But I would like to ask you what the state is of the mentioned test cases? Is it possible to commit those and I'm gonna finish them? |
|
@nivida Ah thanks for the reminder - I will get these in.. |
When using `.on<event>=fn` to attach listeners, only one listener can be set at the same time. Since multiple request managers can use the same provider, the EventTarget API has to be used to ensure all of them receive the events emitted from the provider. This is needed on both the `on` and `removeListener` functions.
The method `once` is required to allow the subscription logic to identify if the provider is able to reconnect/resubscribe and then attach to the following `connect` event the function to resubscribe.
When the subscription fails on start and when it fails after it was successfully established, the same logic needs to be executed: remove subscription, listen for the next `connect` event if available to actually subscribe again, emit the error and call the callback. Prior code did that only for established subscriptions so if a subscription was unable to be set right on start, no resubscription was ever tried. The logic was moved to a single method to avoid duplication of code. In addition reentry is avoided by checking and properly clearing the `_reconnectIntervalId` variable.
On subscribe, if there is an existing `id`, the subscription listeners are removed. In the case of a resubscription, the listeners have to be kept. Therefore, the `id` property -that will change anyway- must be cleared so the listeners are not removed. Then, after the subscription object resubscribes, the listeners set by the subscription user code remain untouched, making the resubscription transparent to the user code.
When the request manager removes a subscription due to an error, it tries to send an unsubscribe package, which can also fail if i.e. the network is down. In such a case, the function must not allow reentry. Removing the subscription first ensures it will not do so. In addition, if the subscription was already removed, the callback shall be called anyway.
When error events are emitted by the provider, all subscriptions shall receive the event and trigger the unsubscription/resubscription logic.
By wrapping the available WebSocket implementation (native WebSocket object or `websocket` package) with `websocket-reconnector`, the provider is given a WebSocket that will automatically reconnect on errors. A new option was added to the WebSocket provider to controll whether it should automatically reconnect or it should behave as usual.
In the case any websocket call takes too long to return and a timeout was set for the provider to timeout, the provider should try to restart the connection. This could happen, for instance, if the client loses connection with the server, the server closes the connection and later, the connectivity is up again but since the client did not receive the closing frame *and* the client does not attempt to send any package to the server, no error is observed. `websocket` implementation for Node.js has an option to send keep-alive frames and detect such scenarios, but the standard browser W3C WebSocket does not, so it is "vulnerable" to this kind of failure which will mostly affect web3 subscriptions.
If the provider silently recoonects and emits a new "connect" event, the subscriptions have to be set again over that new connection.
cgewecke
force-pushed
the
staging/reconnect-resubscribe
branch
from
5c5e95d
to
d31f7e2
Compare
cgewecke
commented
Nov 6, 2019
| }); | ||
|
|
||
| // This test failing.... | ||
| it.only('auto reconnects', function(){ |
There was a problem hiding this comment.
@nivida This is my attempt to model reconnection working.
- Open server
- Attach a subscription handler
- Fire a tx (which subscrip hears)
- Close server
- Re-open server
- Fire a tx (which subscrip hears)
But the test doesn't reconnect...do you want to take a look?
There was a problem hiding this comment.
We need to make sure this works against an actual client, not just mock some assumptions in.
There was a problem hiding this comment.
Maybe my model of how to do this is wrong...not sure.
There was a problem hiding this comment.
@nivida mentioned something in a chat that's important which I'm leaving here for further reference.
The way this test models disconnection doesn't reflect the connectivity issues people typically have online e.g. nodes are not shutting down, killing their sockets and restarting...
1 task
This was referenced Nov 7, 2019
|
Moved this to #3190 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is #3135, #1966 redux, merged to the root repo so merge conflicts and tests can be managed here. Tests are WIP, opening as draft.