Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Fixes 'Connection not open on send' #3171

Closed
wants to merge 2 commits into from
Closed

Fixes 'Connection not open on send' #3171

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a978d8
periodically occuring 'connection not open on send()' fixed
nivida Oct 31, 2019
8715096
Update packages/web3-providers-ws/src/index.js
nivida Nov 7, 2019
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
111 changes: 57 additions & 54 deletions packages/web3-providers-ws/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @date 2017
*/

"use strict";
'use strict';

var _ = require('underscore');
var errors = require('web3-core-helpers').errors;
Expand All @@ -41,8 +41,7 @@ if (isNode) {
parseURL = function(url) {
return new newURL(url);
};
}
else {
} else {
// Web3 supports Node.js 5, so fall back to the legacy URL API if necessary
parseURL = require('url').parse;
}
Expand All @@ -55,9 +54,7 @@ if (isNode) {
// Default connection ws://localhost:8546




var WebsocketProvider = function WebsocketProvider(url, options) {
var WebsocketProvider = function WebsocketProvider(url, options) {
if (!Ws) {
throw new Error('websocket is not available');
}
Expand All @@ -81,7 +78,7 @@ var WebsocketProvider = function WebsocketProvider(url, options) {

// Allow a custom client configuration
var clientConfig = options.clientConfig || undefined;

// Allow a custom request options
// https://github.com/theturtle32/WebSocket-Node/blob/master/docs/WebSocketClient.md#connectrequesturl-requestedprotocols-origin-headers-requestoptions
var requestOptions = options.requestOptions || undefined;
Expand All @@ -101,29 +98,29 @@ var WebsocketProvider = function WebsocketProvider(url, options) {
/*jshint maxcomplexity: 6 */
var data = (typeof e.data === 'string') ? e.data : '';

_this._parseResponse(data).forEach(function(result){
_this._parseResponse(data).forEach(function(result) {

var id = null;

// get the id which matches the returned id
if(_.isArray(result)) {
result.forEach(function(load){
if(_this.responseCallbacks[load.id])
if (_.isArray(result)) {
result.forEach(function(load) {
if (_this.responseCallbacks[load.id])
id = load.id;
});
} else {
id = result.id;
}

// notification
if(!id && result && result.method && result.method.indexOf('_subscription') !== -1) {
_this.notificationCallbacks.forEach(function(callback){
if(_.isFunction(callback))
if (!id && result && result.method && result.method.indexOf('_subscription') !== -1) {
_this.notificationCallbacks.forEach(function(callback) {
if (_.isFunction(callback))
callback(result);
});

// fire the callback
} else if(_this.responseCallbacks[id]) {
} else if (_this.responseCallbacks[id]) {
_this.responseCallbacks[id](null, result);
delete _this.responseCallbacks[id];
}
Expand All @@ -132,26 +129,26 @@ var WebsocketProvider = function WebsocketProvider(url, options) {

// make property `connected` which will return the current connection status
Object.defineProperty(this, 'connected', {
get: function () {
return this.connection && this.connection.readyState === this.connection.OPEN;
},
enumerable: true,
});
get: function() {
return this.connection && this.connection.readyState === this.connection.OPEN;
},
enumerable: true
});
};

/**
Will add the error and end event to timeout existing calls

@method addDefaultEvents
*/
WebsocketProvider.prototype.addDefaultEvents = function(){
WebsocketProvider.prototype.addDefaultEvents = function() {
var _this = this;

this.connection.onerror = function(){
this.connection.onerror = function() {
_this._timeout();
};

this.connection.onclose = function(){
this.connection.onclose = function() {
_this._timeout();

// reset all requests and callbacks
Expand All @@ -175,30 +172,30 @@ WebsocketProvider.prototype._parseResponse = function(data) {

// DE-CHUNKER
var dechunkedData = data
.replace(/\}[\n\r]?\{/g,'}|--|{') // }{
.replace(/\}\][\n\r]?\[\{/g,'}]|--|[{') // }][{
.replace(/\}[\n\r]?\[\{/g,'}|--|[{') // }[{
.replace(/\}\][\n\r]?\{/g,'}]|--|{') // }]{
.replace(/\}[\n\r]?\{/g, '}|--|{') // }{
.replace(/\}\][\n\r]?\[\{/g, '}]|--|[{') // }][{
.replace(/\}[\n\r]?\[\{/g, '}|--|[{') // }[{
.replace(/\}\][\n\r]?\{/g, '}]|--|{') // }]{
.split('|--|');

dechunkedData.forEach(function(data){
dechunkedData.forEach(function(data) {

// prepend the last chunk
if(_this.lastChunk)
if (_this.lastChunk)
data = _this.lastChunk + data;

var result = null;

try {
result = JSON.parse(data);

} catch(e) {
} catch (e) {

_this.lastChunk = data;

// start timeout to cancel all requests
clearTimeout(_this.lastChunkTimeout);
_this.lastChunkTimeout = setTimeout(function(){
_this.lastChunkTimeout = setTimeout(function() {
_this._timeout();
throw errors.InvalidResponse(data);
}, 1000 * 15);
Expand All @@ -210,7 +207,7 @@ WebsocketProvider.prototype._parseResponse = function(data) {
clearTimeout(_this.lastChunkTimeout);
_this.lastChunk = null;

if(result)
if (result)
returnValues.push(result);
});

Expand All @@ -235,7 +232,7 @@ WebsocketProvider.prototype._addResponseCallback = function(payload, callback) {

// schedule triggering the error response if a custom timeout is set
if (this._customTimeout) {
setTimeout(function () {
setTimeout(function() {
if (_this.responseCallbacks[id]) {
_this.responseCallbacks[id](errors.ConnectionTimeout(_this._customTimeout));
delete _this.responseCallbacks[id];
Expand All @@ -250,36 +247,42 @@ WebsocketProvider.prototype._addResponseCallback = function(payload, callback) {
@method _timeout
*/
WebsocketProvider.prototype._timeout = function() {
for(var key in this.responseCallbacks) {
if(this.responseCallbacks.hasOwnProperty(key)){
for (var key in this.responseCallbacks) {
if (this.responseCallbacks.hasOwnProperty(key)) {
this.responseCallbacks[key](errors.InvalidConnection('on WS'));
delete this.responseCallbacks[key];
}
}
};


WebsocketProvider.prototype.send = function (payload, callback) {
WebsocketProvider.prototype.send = function(payload, callback) {
var _this = this;

if (this.connection.readyState === this.connection.CONNECTING) {
setTimeout(function () {
_this.send(payload, callback);
}, 10);
this.connection.addEventListener(
'open',
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if this is never fired because the client is not available, for example when testing against a local node the user forgot to turn on? In the current flow it looks like it will send and error, but here maybe hang?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true it will with the changed lines of code just hang until the connection got established and in the case of a non-existing host will it never execute the JSON-RPC methods we have in the pipeline. The emitted error net::ERR_CONNECTION_REFUSED which would signal us the non-existing host. Can't get caught with a try/catch in the WebsocketProvider constructor because it doesn't throw an error in the normal sense. The WebSocket class does dispatch a non-listenable event because of the way it is implemented. A solution would be to create a connecting-timeout function in the constructor of the WebsocketProvider which throws an error if the connection doesn't get established in the defined time frame.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A solution would be to create a connecting-timeout function in the constructor of the WebsocketProvider which throws an error if the connection doesn't get established in the defined time frame.

Great analysis! To me it seems like there are two execution contexts for Web3 with opposite expectations:

  • NodeJS (testing/tools) --> fail fast and hard.
  • Network (browser/dapps) --> keep trying for as long as it takes

At the moment, web3 behaves mostly the way NodeJS contexts would prefer, and there are lots of complaints on the browser / dapp side. Maybe we can just acknowledge the difference in the docs/release notes and address it by toggling the different behaviors via the autoReconnect: true (or something similar) proposed in #3167. Here you might just check that flag - stay open if true, fail fast as Web3 currently does if false?

WDYT? This change could be paired with #3167...

function() {
_this.send(payload, callback);
},
{once: true}
);

return;
}

// try reconnect, when connection is gone
// if(!this.connection.writable)
// this.connection.connect({url: this.url});
if (this.connection.readyState !== this.connection.OPEN) {
console.error('connection not open on send()');
if (typeof this.connection.onerror === 'function') {
this.connection.onerror(new Error('connection not open'));
this.connection.onerror(new Error('connection not open on send()'));
} else {
console.error('no error callback');
}
callback(new Error('connection not open'));

callback(new Error('connection not open on send()'));

return;
}

Expand All @@ -294,12 +297,12 @@ WebsocketProvider.prototype.send = function (payload, callback) {
@param {String} type 'notifcation', 'connect', 'error', 'end' or 'data'
@param {Function} callback the callback to call
*/
WebsocketProvider.prototype.on = function (type, callback) {
WebsocketProvider.prototype.on = function(type, callback) {

if(typeof callback !== 'function')
if (typeof callback !== 'function')
throw new Error('The second parameter callback must be a function.');

switch(type){
switch (type) {
case 'data':
this.notificationCallbacks.push(callback);
break;
Expand Down Expand Up @@ -331,13 +334,13 @@ WebsocketProvider.prototype.on = function (type, callback) {
@param {String} type 'notifcation', 'connect', 'error', 'end' or 'data'
@param {Function} callback the callback to call
*/
WebsocketProvider.prototype.removeListener = function (type, callback) {
WebsocketProvider.prototype.removeListener = function(type, callback) {
var _this = this;

switch(type){
switch (type) {
case 'data':
this.notificationCallbacks.forEach(function(cb, index){
if(cb === callback)
this.notificationCallbacks.forEach(function(cb, index) {
if (cb === callback)
_this.notificationCallbacks.splice(index, 1);
});
break;
Expand All @@ -356,8 +359,8 @@ WebsocketProvider.prototype.removeListener = function (type, callback) {
@method removeAllListeners
@param {String} type 'notifcation', 'connect', 'error', 'end' or 'data'
*/
WebsocketProvider.prototype.removeAllListeners = function (type) {
switch(type){
WebsocketProvider.prototype.removeAllListeners = function(type) {
switch (type) {
case 'data':
this.notificationCallbacks = [];
break;
Expand Down Expand Up @@ -387,7 +390,7 @@ WebsocketProvider.prototype.removeAllListeners = function (type) {

@method reset
*/
WebsocketProvider.prototype.reset = function () {
WebsocketProvider.prototype.reset = function() {
this._timeout();
this.notificationCallbacks = [];

Expand All @@ -398,7 +401,7 @@ WebsocketProvider.prototype.reset = function () {
this.addDefaultEvents();
};

WebsocketProvider.prototype.disconnect = function () {
WebsocketProvider.prototype.disconnect = function() {
if (this.connection) {
this.connection.close();
}
Expand All @@ -410,7 +413,7 @@ WebsocketProvider.prototype.disconnect = function () {
* @method supportsSubscriptions
* @returns {boolean}
*/
WebsocketProvider.prototype.supportsSubscriptions = function () {
WebsocketProvider.prototype.supportsSubscriptions = function() {
return true;
};

Expand Down