-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support websocket reconnect on web3-providers-ws. #1851
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that this code works. Adding a test that shows that it works would help me in my confidence. Also I think #1852 is not an issue that should be accepted. It is okay to throw an error when the websocket connection fails and let the user handle the problem upstream (by recreating the provider for example)
@@ -76,6 +76,7 @@ var WebsocketProvider = function WebsocketProvider(url, options) { | |||
} | |||
|
|||
this.connection = new Ws(url, protocol, undefined, headers); | |||
this.reconnect = () => new Ws(url, protocol, undefined, headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think you must bind the result of this.reconnect()
to this.connection
. If not then you are awkwardly relying on some side-effect from the Websockets constructor call. My bet is: This does not work at all. Have you tested it?
My suggestion: Add an automated test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes binding
will be good. Thanks :)
callback(new Error('connection not open')); | ||
// try reconnect, when connection is gone | ||
this.reconnect(); | ||
callback(new Error('connection not open. try reconnecting...')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of throwing an error when at the same trying to solve it. You should silently retry until you are sure there is no way to recover. Then you should stop trying to reconnect and throw an error. This here does not make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a need to inform web3.js and the developer, as subscription are socket bound, if the socket breaks and auto-reconnects, developers will wonder why they never get their subscriptions again, as it was basically canceled without them knowing. Also web3.js has some logic of re-subscribing. Please check if that still works! https://github.com/ethereum/web3.js/blob/1.0/packages/web3-core-subscriptions/src/subscription.js#L278-L289
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frozeman Thanks! I'll check it :)
I've closed this PR because of #1966 |
Description
Support websocket reconnect, when connection is closed.
Type of change
Checklist:
npm run test
with success and extended the tests if necessary.npm run build
and tested the resulting file fromdist
folder in a browser.#fix #1558 #1852