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

Feature/improve xml request error handling (#3947) #4298

Closed
wants to merge 4 commits into from
Closed

Feature/improve xml request error handling (#3947) #4298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f5b63a
Feature/improve xml request error handling (#3947)
uluhonolulu Sep 8, 2021
a1c0078
Update CHANGELOG.md
spacesailor24 Sep 8, 2021
1e92833
Update if statement to be less explicit
spacesailor24 Sep 14, 2021
c53c752
Merge branch '1.x' into wyatt/3947-xml-error-handling
spacesailor24 Sep 14, 2021
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,4 +447,5 @@ Released with 1.0.0-beta.37 code base.
- lerna from 3.22.1 to 4.0.0 (#4231)
- Dropped build tests in CI for Node v8 and v10, and added support for Node v14
- Change default value for `maxPriorityFeePerGas` from `1 Gwei` to `2.5 Gwei` (#4284)
- Fixed bug in signTransaction (#4295)
- Fixed bug in signTransaction (#4295)
- Fixed misleading error thrown on XMLHTTP request error (#4298)
3 changes: 3 additions & 0 deletions packages/web3-core-helpers/src/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ module.exports = {
var message = !!result && !!result.error && !!result.error.message ? result.error.message : 'Invalid JSON RPC response: ' + JSON.stringify(result);
return new Error(message);
},
RequestFailed: function (){
return new Error('HTTP request failed');
},
ConnectionTimeout: function (ms){
return new Error('CONNECTION TIMEOUT: timeout of ' + ms + ' ms achived');
},
Expand Down
14 changes: 12 additions & 2 deletions packages/web3-providers-http/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ HttpProvider.prototype.send = function (payload, callback) {
var request = this._prepareRequest();
Copy link
Contributor

Choose a reason for hiding this comment

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

@spacesailor24 The PR referred in mentioned to upgrade xhr2-cookies to node-xhr2. I could not find that change incorporated here. Does the scope for this PR changed?

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 PR does not include the package switch. What do y'all think about updating the package used?

Current package - Last release 4 years ago
Suggested package - Last release was Feb 9 2021

CC @jdevcs @luu-alex

Copy link
Contributor

Choose a reason for hiding this comment

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

@spacesailor24 could you check xhr2-cookies vs node-xhr2 with internal Lib Selection Checklist for Web3.js .
( build size impact, ..etc ) , lets open another PR for lib switch, if this lib update is not directly required for fix of xml request error handling ( this PR )

Copy link

Choose a reason for hiding this comment

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

@spacesailor24 BTW node-xhr2 has the same issue (https://github.com/pwnall/node-xhr2/blob/master/src/001-xml_http_request.coffee#L608), but at least it's developed more actively so there's a chance they fix it.

As far as I see, the code looks pretty similar to xhr2-cookies, just rewritten in CoffeeScript.


request.onreadystatechange = function() {
if (request.readyState === 4 && request.timeout !== 1) {
if (request.readyState === 4 && request.timeout !== 1 && request.status !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As per documentation this property returns (HTTP Status Codes)[https://developer.mozilla.org/en-US/docs/Web/HTTP/Status] and there is no status code 0 exists, so why are we checking it here?

Do we want to check if the status code exists? if yes then condition should not use a constant zero.

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mentioned in the related issue, request.status is 0. Also mentioned here

var result = request.responseText;
var error = null;

Expand All @@ -112,6 +112,16 @@ HttpProvider.prototype.send = function (payload, callback) {
}
};

//since XHR2._onHttpRequestError swallows the initial request error, we need to get it from the underlying request
request.addEventListener('loadstart', function() {
var clientRequest = request._request;
Copy link
Contributor

Choose a reason for hiding this comment

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

If request is an object of XMLHttpRequest then I could not find any documented reference of _request property. We should avoid using such private behaviour of a native object.

Also the request object could be either xhr2-cookies or XMLHttpRequest so such private attributes can't provide confirmation of availability on different type of objects.

Did you tried top level events error or abort? As per docs if there is any error error should be fired after loadstart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_request is a private property, and this PR would likely break if we switch out the package to use node-xhr2

This PR was originally created by @uluhonolulu, and I'm not really sure how to setup an environment to test that this works as intended. However, how we adjust this specific code is dependent on what package we choose to go with

Copy link
Contributor

Choose a reason for hiding this comment

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

why we are binding clientRequest.on('error'... when request started loading loadstart ? shouldnt it be done directly with request inside HttpProvider.prototype.send ?

Copy link

@uluhonolulu uluhonolulu Sep 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If request is an object of XMLHttpRequest then I could not find any documented reference of _request property. We should avoid using such private behaviour of a native object.

Also the request object could be either xhr2-cookies or XMLHttpRequest so such private attributes can't provide confirmation of availability on different type of objects.

Did you tried top level events error or abort? As per docs if there is any error error should be fired after loadstart

@nazarhussain this is my hacky attempt to get the underlying error, because xhr2-cookies just swallows it (see souldreamer/xhr2-cookies#9).

Copy link

Choose a reason for hiding this comment

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

why we are binding clientRequest.on('error'... when request started loading loadstart ? shouldnt it be done directly with request inside HttpProvider.prototype.send ?

@jdevcs because request._request doesn't exist at this point yet.

if (clientRequest) {
clientRequest.on('error', function (error) {
callback(error || errors.RequestFailed());
});
}
});

request.ontimeout = function() {
_this.connected = false;
callback(errors.ConnectionTimeout(this.timeout));
Expand All @@ -121,7 +131,7 @@ HttpProvider.prototype.send = function (payload, callback) {
request.send(JSON.stringify(payload));
} catch(error) {
this.connected = false;
callback(errors.InvalidConnection(this.host));
callback(errors.InvalidConnection(this.host, { code: 'ECONNREFUSED', reason: 'ECONNREFUSED' }));
}
};

Expand Down
5 changes: 5 additions & 0 deletions test/helpers/FakeXHR2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var assert = chai.assert;
var FakeXHR2 = function () {
this.responseText = undefined;
this.readyState = 4;
this.status = 200;
spacesailor24 marked this conversation as resolved.
Show resolved Hide resolved
this.onreadystatechange = null;
this.async = true;
this.agents = {};
Expand Down Expand Up @@ -38,4 +39,8 @@ FakeXHR2.prototype.send = function (payload) {
}
};

FakeXHR2.prototype.addEventListener = function(eventType, callback) {
callback();
}

module.exports = {XMLHttpRequest: FakeXHR2};