Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support websocket reconnect on web3-providers-ws. #1851

Closed
wants to merge 4 commits into from
Closed

Support websocket reconnect on web3-providers-ws. #1851

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa6c74b
Fix bug: Call 'getGasPrice' rpc when tx.gasPrice is set to 0 intentio…
nujabes403 Aug 5, 2018
d034858
Compare whether the value is undefined through 'typeof' operator
nujabes403 Aug 6, 2018
6966cac
Support websocket reconnect on web3-providers-ws
nujabes403 Aug 7, 2018
bcacaeb
Merge branch '1.0' into 1.0
nujabes403 Aug 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/web3-providers-ws/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

Copy link
Contributor Author

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 :)


this.addDefaultEvents();

Expand Down Expand Up @@ -263,7 +264,9 @@ WebsocketProvider.prototype.send = function (payload, callback) {
} else {
console.error('no error callback');
}
callback(new Error('connection not open'));
// try reconnect, when connection is gone
this.reconnect();
callback(new Error('connection not open. try reconnecting...'));
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

Copy link
Contributor

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

Copy link
Contributor Author

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 :)

return;
}

Expand Down