Provider related improvements #3190
Conversation
… of a setRequestManager method
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.
Co-Authored-By: cgewecke <christophergewecke@gmail.com>
…cribing after the connection got lost will now get triggered from the RequestManager instead of the subscriptions
|
Open discussion from PR #3171: |
|
@nivida Thanks for the great work! |
|
@svanurin Won't make any promises here. If we get this merged next week during EthCC this will get its way into the following |
|
@holgerd77 I see. Thank you and good luck with your work! |
Document WS clientConfig options in README and RTD
|
@holgerd77 Rebased against latest 1.x now. One of the "allowed failure" jobs (ganache-core's unit tests) is failing on a single test. This has already been reported/discussed in ganache-core 545 and they agree it's a bug in their test / websocket disconnection logic. |
|
Thanks for all the work that went into this! This has been a blocker to upgrading for me, blocking being able to report and/or fix issues in the latest. |
|
Hi @wbt. We're working on releasing And |
|
WHEN IS 1.2.7 GONNA BE RELEASED JESUS CHRIST THIS IS A MAJOR BUG |
|
@dr3adx This work is published in 1.2.7-rc.0 and we should cut a formal release for that this week. If you'd like to try it out right now you can install with |
|
@dr3adx what issues do you see? |
|
@cgewecke do I understand this PR right and it removes the ability to give each module a different provider connection? |
|
@frozeman, thanks for checking this over...
This topic was raised back in November when the change was first proposed in this thread. There it seemed like the main concern was that the If you have a chance could you clarify what the expected behavior is with an example? |
|
@nivida seems like the providers provider = new Web3.providers.WebsocketProvider(url);
provider.on('end', () => {
// not fired
});
provider.on('error', () => {
// not fired
});
contract.events[type]({ fromBlock }).on('error', (err) => {
// event fired
})
provider.disconnect(1012, 'restart'); |
Description
Fixes #1648, #3042, #3092, #1085, #1391, #1558, #1852, #1646, #1510 and #1933
Because PR #3187, #3156, #3171, and #3167 do all depend strongly on each other and it's not possible to finish them separately. Did we move all of those PRs together and do improve and extend the provider handling of web3.js here in one bigger PR.
RequestManager
The provider will now only get resolved once and the
datalistener is no longer added on each initiation of aModule,Contract, or on executing of thesetProvidermethod on each package.Old Behaviour
setProvider.datalistener.setProvideron all children's.datalistener for each child package.New Behaviour
setProvider.datalistener.setRequestManageron all children's.By side of improving the inheritance logic of the
RequestManagerdid we add theerrorandconnectevent listener to pass errors tosubscriptionsand to callresubscribeon eachsubscriptionif the currently used provider has re-established his connection.WebsocketProvider
connection not open error:
Instead of just waiting 10ms with a setTimeout and to try it again does it now add the item to the
requestQueueand will get executed as soon as theconnectevent does get emitted. If the connection does emit anerrororcloseevent does it terminate the request correctly as well.Reconnecting:
Options:
Websocket reconnecting is a long-standing issue in the 1.x branch of this library and is the source of many issues when developing applications that for any reason use a not-so-reliable connection to the nodes.
Implementing of the reconnect handling means basically resolving several major problems:
Those fixes do change the following:
RequestManager's provider listeners updated:reconnectmethod implementedType of change
Checklist:
npm run testwith success and extended the tests if necessary.npm run buildand tested the resulting file fromdistfolder in a browser.