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

spacesailor24 · 2021-09-08T02:21:49Z

I took over #3947 to fix CHANGELOG.md

* Improve http request error handling * Update changelog.md * Update mock response * Add error code to HTTP connection error * Propagate the original request error (#3425) * Fixed a test (#3425) Co-authored-by: Liam Aharon <[email protected]>

render · 2021-09-08T02:21:53Z

Your Render PR Server URL is https://web3-js-pr-4298.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c4s1sfs1nok4ouigmojg.

coveralls · 2021-09-08T02:36:44Z

Pull Request Test Coverage Report for Build 1232103545

0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
11 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.05%) to 75.438%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/web3-core-helpers/src/errors.js	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
packages/web3-core-helpers/src/errors.js	1	1.54%
packages/web3-core-helpers/lib/errors.js	4	86.96%
packages/web3-providers-http/lib/index.js	6	74.44%

Totals
Change from base Build 1222417029:	-0.05%
Covered Lines:	3199
Relevant Lines:	4015

💛 - Coveralls

packages/web3-providers-http/src/index.js

test/helpers/FakeXHR2.js

spacesailor24 · 2021-09-15T03:19:18Z

For better understanding, we can see that on line 227 the xhr2-cookies package calls it's _sendHxxpRequest, which then has a .on('error'... that calls _onHttpRequestError on line 252

And here we can see that _onHttpRequestError does nothing with error, so it does appear to be swallowing any Http errors

spacesailor24 · 2021-09-15T03:25:08Z

@uluhonolulu Did you test that these changes solve #3425? Would it be possible for you to provide an example project where an xhr2-cookies is caught and emitted from the changes you made? I'm just having a hard time understanding this enough to merge it

nazarhussain · 2021-09-15T12:51:50Z

packages/web3-providers-http/src/index.js

@@ -97,7 +97,7 @@ HttpProvider.prototype.send = function (payload, callback) {
    var request = this._prepareRequest();

    request.onreadystatechange = function() {
-        if (request.readyState === 4 && request.timeout !== 1) {
+        if (request.readyState === 4 && request.timeout !== 1 && request.status !== 0) {


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

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

nazarhussain · 2021-09-15T12:55:28Z

packages/web3-providers-http/src/index.js

@@ -97,7 +97,7 @@ HttpProvider.prototype.send = function (payload, callback) {
    var request = this._prepareRequest();


@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?

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

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

@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.

nazarhussain · 2021-09-15T13:00:06Z

packages/web3-providers-http/src/index.js

@@ -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;


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

_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

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

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

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.

uluhonolulu · 2021-09-19T18:06:50Z

And here we can see that _onHttpRequestError does nothing with error, so it does appear to be swallowing any Http errors

Exactly. This is why I suggested switching to node-xhr2.

uluhonolulu · 2021-09-19T18:16:55Z

@uluhonolulu Did you test that these changes solve #3425? Would it be possible for you to provide an example project where an xhr2-cookies is caught and emitted from the changes you made? I'm just having a hard time understanding this enough to merge it

@spacesailor24 I tested it with my own project and I'm 100% sure it worked as intended, but it was half a year ago so I don't even remember how I invoked the error. Most probably I just tried to connect to a node that didn't exist.

I can provide a test project, but TBH I'm in a middle of moving to a new apt so it's going to be the next weekend at best.

spacesailor24 · 2021-09-19T19:56:38Z

@uluhonolulu Did you test that these changes solve #3425? Would it be possible for you to provide an example project where an xhr2-cookies is caught and emitted from the changes you made? I'm just having a hard time understanding this enough to merge it

@spacesailor24 I tested it with my own project and I'm 100% sure it worked as intended, but it was half a year ago so I don't even remember how I invoked the error. Most probably I just tried to connect to a node that didn't exist.

I can provide a test project, but TBH I'm in a middle of moving to a new apt so it's going to be the next weekend at best.

@uluhonolulu okay! no worries on the test project, I'll try connecting to a node that doesn't exist. Thank you for your patience on getting this merged, apologies for it taking so long!

spacesailor24 · 2021-09-23T03:32:58Z

I used the following code to produce the below errors (left terminal is 1.x, right is with this patch):

const Web3 = require('web3');

(async () => {
    const web3 = new Web3('http://127.0.0.1');
    try {
        const result = await web3.eth.getBlock(1);
    } catch (error) {
        console.log(error)
    }
})()

spacesailor24 · 2021-09-23T04:02:14Z

My opinion on this is that we shouldn't even be using an XML HTTP package and should rely either on the native Node.js HTTP package or something like Fetch or Axios. However, as most of our tests make use of the FakeHttpProvider, and we don't have many tests actually executing HTTP requests, I don't think it would be wise to make changes to what's probably the most used protocol for Web3.js (unless it's for major bugs or security patches). Instead, I suggest waiting for the rewrite (4.x) to be completed, where this will be handled in a more modern way; if we're not merging this PR as is for the immediate benefit

valamidev · 2021-10-02T06:57:52Z

My opinion on this is that we shouldn't even be using an XML HTTP package and should rely either on the native Node.js HTTP package or something like Fetch or Axios. However, as most of our tests make use of the FakeHttpProvider, and we don't have many tests actually executing HTTP requests, I don't think it would be wise to make changes to what's probably the most used protocol for Web3.js (unless it's for major bugs or security patches). Instead, I suggest waiting for the rewrite (4.x) to be completed, where this will be handled in a more modern way; if we're not merging this PR as is for the immediate benefit

Well xhr2-cookies were last updated 4 years ago and clearly cause a lot of trouble with his error handling / 'hiding' . When I discovered that my Invalid JSON RPC response: "" error origin is coming from an expired CERT...

Error: certificate has expired
    at TLSSocket.onConnectSecure (_tls_wrap.js:1507:34)
    at TLSSocket.emit (events.js:376:20)
    at TLSSocket._finishInit (_tls_wrap.js:932:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:706:12) {
  code: 'CERT_HAS_EXPIRED'

First it is a false error, because the response was not invalid it was throwed by an error, second I never expected that in case of HTTP error I won't get any notification.
I am happy to see that the issue is already have an ongoing PR, but IMO it should be in the mid priority stuffs or even high.

jdevcs · 2021-10-20T17:42:03Z

Thanks for bringing this discussion in 1.x, our team will consider this request error handling improvement in web3.js 4.x.

github-actions · 2022-02-10T00:15:50Z

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

jdevcs · 2022-02-17T14:47:50Z

Closing this in favor of 4.x

Thanks for bringing this discussion in 1.x, our team will consider this request error handling improvement in web3.js 4.x.

spacesailor24 self-assigned this Sep 8, 2021

spacesailor24 commented Sep 8, 2021

View reviewed changes

packages/web3-providers-http/src/index.js Outdated Show resolved Hide resolved

spacesailor24 added 3 commits September 7, 2021 21:42

Update CHANGELOG.md

a1c0078

Update if statement to be less explicit

1e92833

Merge branch '1.x' into wyatt/3947-xml-error-handling

c53c752

spacesailor24 commented Sep 15, 2021

View reviewed changes

test/helpers/FakeXHR2.js Show resolved Hide resolved

nazarhussain suggested changes Sep 15, 2021

View reviewed changes

jdevcs linked an issue Sep 16, 2021 that may be closed by this pull request

XMLHttpRequest errors are not handled correctly #3425

Closed

jdevcs mentioned this pull request Sep 23, 2021

xhr2-cookies vs node-xhr2 #4346

Closed

github-actions bot added the Stale Has not received enough activity label Feb 10, 2022

jdevcs closed this Feb 17, 2022

spacesailor24 deleted the wyatt/3947-xml-error-handling branch August 4, 2022 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

spacesailor24 commented Sep 8, 2021

render bot commented Sep 8, 2021

coveralls commented Sep 8, 2021 •

edited

Loading

spacesailor24 commented Sep 15, 2021

spacesailor24 commented Sep 15, 2021

nazarhussain Sep 15, 2021

spacesailor24 Sep 15, 2021

nazarhussain Sep 15, 2021

spacesailor24 Sep 15, 2021

jdevcs Sep 16, 2021

uluhonolulu Sep 19, 2021

nazarhussain Sep 15, 2021

spacesailor24 Sep 15, 2021

jdevcs Sep 16, 2021

uluhonolulu Sep 19, 2021 •

edited

Loading

uluhonolulu Sep 19, 2021

uluhonolulu commented Sep 19, 2021

uluhonolulu commented Sep 19, 2021

spacesailor24 commented Sep 19, 2021

spacesailor24 commented Sep 23, 2021

spacesailor24 commented Sep 23, 2021 •

edited

Loading

valamidev commented Oct 2, 2021

jdevcs commented Oct 20, 2021

github-actions bot commented Feb 10, 2022

jdevcs commented Feb 17, 2022

		@@ -97,7 +97,7 @@ HttpProvider.prototype.send = function (payload, callback) {
		var request = this._prepareRequest();

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

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

Conversation

spacesailor24 commented Sep 8, 2021

render bot commented Sep 8, 2021

coveralls commented Sep 8, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1232103545

💛 - Coveralls

spacesailor24 commented Sep 15, 2021

spacesailor24 commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uluhonolulu Sep 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uluhonolulu commented Sep 19, 2021

uluhonolulu commented Sep 19, 2021

spacesailor24 commented Sep 19, 2021

spacesailor24 commented Sep 23, 2021

spacesailor24 commented Sep 23, 2021 • edited Loading

valamidev commented Oct 2, 2021

jdevcs commented Oct 20, 2021

github-actions bot commented Feb 10, 2022

jdevcs commented Feb 17, 2022

coveralls commented Sep 8, 2021 •

edited

Loading

uluhonolulu Sep 19, 2021 •

edited

Loading

spacesailor24 commented Sep 23, 2021 •

edited

Loading