XMLHttpRequest errors are not handled correctly #3425

liamaharon · 2020-03-22T10:33:57Z

In the event of an error occurring during a XMLHttpRequest, onreadystatechange is called when

request.readyState is 4
request.responseText is ""
request.status is 0

The following code that handles http requests does not handle this case, and can return a misleading error to the user when it occurs

https://github.com/ethereum/web3.js/blob/dee72e00883c5a23a47b6d3c1d27304ab1d4922e/packages/web3-providers-http/src/index.js#L87-L125

Let's pretend a machine has exhausted all of it's ephemeral ports right before web3.js tries to send a request. Here's what happens

HttpProvider.prototype.send is called
The request is crafted
onreadystatechange and ontimeout handlers are assigned
request.send is called with the payload
The request fails before it is even sent to the destination due to there being no ephemeral ports available
request.readyState is set to 4, request.responseText is set to "", request.status is set to 0 and the onreadystatechange handler is called
web3.js tries to parse "" and throws an error that looks like this

  <rejected> Error: Invalid JSON RPC response: ""
      at Object.InvalidResponse (/home/liam/spells/web3.js/packages/web3-core-helpers/src/errors.js:42:16)
      at XMLHttpRequest.request.onreadystatechange (/home/liam/spells/web3.js/packages/web3-providers-http/src/index.js:108:32)
      at XMLHttpRequestEventTarget.dispatchEvent (/home/liam/spells/web3.js/packages/web3-providers-http/node_modules/xhr2-cookies/dist/xml-http-request-event-target.js:34:22)
      at XMLHttpRequest._setReadyState (/home/liam/spells/web3.js/packages/web3-providers-http/node_modules/xhr2-cookies/dist/xml-http-request.js:208:14)
      at XMLHttpRequest._onHttpRequestError (/home/liam/spells/web3.js/packages/web3-providers-http/node_modules/xhr2-cookies/dist/xml-http-request.js:349:14)
      at ClientRequest.<anonymous> (/home/liam/spells/web3.js/packages/web3-providers-http/node_modules/xhr2-cookies/dist/xml-http-request.js:252:61)
      at ClientRequest.emit (events.js:210:5)
      at ClientRequest.EventEmitter.emit (domain.js:475:20)
      at Socket.socketErrorListener (_http_client.js:406:9)
      at Socket.emit (events.js:210:5)
      at Socket.EventEmitter.emit (domain.js:475:20)
      at emitErrorNT (internal/streams/destroy.js:92:8)
      at emitErrorAndCloseNT (internal/streams/destroy.js:60:3)
      at processTicksAndRejections (internal/process/task_queues.js:80:21)

this error is misleading because it indicates that the node successfully received the request and actually returned an empty string in response, when in reality the request never even made it to the node and an empty string is just the default value set on request in an error scenario.

I believe this may be the cause of some other issues people have opened in the past like #3370 and #1919

I think it makes sense to at least add an onerror handler to the request, and in the case that a request fails the error thrown indicates that instead of a misleading InvalidResponse error.

I'm working on a PR for this.

The text was updated successfully, but these errors were encountered:

cgewecke · 2020-03-24T15:14:36Z

@liamaharon Thanks for such a great description and analysis of this problem.

wbt · 2020-04-27T00:32:37Z

As an FYI: With the error lacking any particular code to check on, we and probably others are checking for a string match and when the error message changes to something more informative, that change will probably break our ability to handle it robustly (mainly by waiting a bit and trying again). We will need to coordinate code changes around the time of upgrade to adjust for that breaking change. Therefore, I would definitely recommend against rolling out a fix for this on e.g. the patch version number.

github-actions · 2020-07-03T00:34:45Z

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment, otherwise this issue will be closed in 7 days

wbt · 2020-07-03T11:50:25Z

I don't think this should be marked as stale.

github-actions · 2020-08-03T00:41:13Z

This issue has been automatically marked as stale because 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.

wbt · 2020-08-03T18:00:53Z

I still don't think this should be marked as stale.

### Description - This change should fix the flakey `error: Invalid JSON RPC response: ""` error that we often see in the protocol test beforeEach hooks (particularly those for the Attestations contract). - Implements a custom beforeEachWithRetries hook for the protocol tests package that will optionally retry beforeEach hooks and add sleep in between retries. This is necessary to avoid flakiness that seems to be caused by port exhaustion in CI and a mishandling of the error by web3. (See web3/web3.js#3425 and web3/web3.js#926). - I've only added `beforeEachWithRetries` to the Attestations tests because this error appears there the most. We can add it elsewhere in the future as needed. ### Other changes None ### Tested To see that this works, search logs for "retry" in https://circleci.com/gh/celo-org/celo-monorepo/248635 ### Related issues - Fixes #4570 ### Backwards compatibility Not applicable

uluhonolulu · 2020-09-26T17:01:30Z

I wish I could do something to help with this, I keep seeing this error and have to write dirty workaround code to handle it every time.

### Description - This change should fix the flakey `error: Invalid JSON RPC response: ""` error that we often see in the protocol test beforeEach hooks (particularly those for the Attestations contract). - Implements a custom beforeEachWithRetries hook for the protocol tests package that will optionally retry beforeEach hooks and add sleep in between retries. This is necessary to avoid flakiness that seems to be caused by port exhaustion in CI and a mishandling of the error by web3. (See web3/web3.js#3425 and web3/web3.js#926). - I've only added `beforeEachWithRetries` to the Attestations tests because this error appears there the most. We can add it elsewhere in the future as needed. ### Other changes None ### Tested To see that this works, search logs for "retry" in https://circleci.com/gh/celo-org/celo-monorepo/248635 ### Related issues - Fixes celo-org#4570 ### Backwards compatibility Not applicable

github-actions · 2020-11-26T00:47:45Z

This issue has been automatically marked as stale because 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.

liamaharon · 2020-11-26T23:00:19Z

@uluhonolulu I have a PR in progress that addresses this, TODO is to get error codes thrown instead of strings. I don't have time to get around to finishing it right now, if you're still willing & able to help would be great if you could assist finishing it off #3426

uluhonolulu · 2020-11-27T06:26:12Z

@liamaharon I'll be happy to work on it. Do you think I'll be able to continue working on your PR (permissions?) or create a new one? (Sorry if it's a dumb question)

github-actions · 2021-01-27T01:01:01Z

This issue has been automatically marked as stale because 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.

wbt · 2021-01-27T17:31:24Z

@liamaharon Did you wind up being able to make any progress? It looks like you got an answer to that question on the PR!

uluhonolulu · 2021-03-05T19:16:21Z

@spacesailor24 I promised to work on it, and I was planning to get it done over the weekend.

Added the custom error

illuzen · 2021-04-22T08:59:41Z

The workaround here is to use WebsocketProvider, at least in the case of truffle tests

github-actions · 2021-07-12T00:09:30Z

This issue has been automatically marked as stale because 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.

wbt · 2021-07-13T15:05:40Z

Is the fix for this still in progress?

uluhonolulu · 2021-07-13T18:09:11Z

@wbt I've created a pull request (#3947), but it didn't pass all tests (the same tests were failing before my changes), but then I stopped receiving notifications and missed the discussion there.

Not sure what is the story with the @koraykoska's fix.

* 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]>

github-actions · 2021-09-12T00:12:05Z

This issue has been automatically marked as stale because 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.

philknows · 2021-11-02T18:18:25Z

Will be addressed in 4.x http provider.

zon-dtravel · 2022-04-26T09:15:48Z

I still got same error when use HttpProvider, any suggest?
this.web3 = new Web3(new Web3.providers.HttpProvider('provider'))

ghost · 2022-07-05T07:56:34Z

Note that this issue has been addressed by the recent PR #5179 to replace the broken xhr2-cookies for the modern fetch API.

liamaharon mentioned this issue Mar 22, 2020

Feature/improve xml request error handling #3426

Closed

13 tasks

cgewecke added the 1.x 1.0 related issues label Mar 23, 2020

benjamincburns mentioned this issue Mar 24, 2020

Ethereum adapter needs better nonce management hyperledger-caliper/caliper#776

Closed

nivida added the Bug Addressing a bug label Mar 26, 2020

cgewecke mentioned this issue Apr 25, 2020

Error: Invalid JSON RPC response: "" #3370

Closed

This was referenced Jun 9, 2020

Add more error details of net or other #3201

Closed

Version 1.2.0 shows different error message when rpc endpoint can't be reached: Error: Invalid JSON RPC response: "" #2988

Closed

boolafish mentioned this issue Jun 26, 2020

fix truffle test failure omgnetwork/plasma-contracts#652

Closed

github-actions bot added the Stale Has not received enough activity label Jul 3, 2020

cgewecke removed the Stale Has not received enough activity label Jul 3, 2020

github-actions bot added the Stale Has not received enough activity label Aug 3, 2020

github-actions bot removed the Stale Has not received enough activity label Aug 4, 2020

alecps mentioned this issue Aug 8, 2020

flakey protocol test beforeEach fix celo-org/celo-monorepo#4651

Merged

github-actions bot added the Stale Has not received enough activity label Nov 26, 2020

github-actions bot removed the Stale Has not received enough activity label Nov 27, 2020

github-actions bot added the Stale Has not received enough activity label Jan 27, 2021

spacesailor24 added the P2 Medium severity bugs label Mar 5, 2021

uluhonolulu added a commit to uluhonolulu/web3.js that referenced this issue Mar 10, 2021

Propagate the original request error (web3#3425)

0f518c9

uluhonolulu added a commit to uluhonolulu/web3.js that referenced this issue Mar 11, 2021

Fixed a test (web3#3425)

183cdda

uluhonolulu mentioned this issue Mar 11, 2021

Feature/improve xml request error handling #3947

Merged

14 tasks

koraykoska added this to the Maintenance milestone Mar 24, 2021

koraykoska self-assigned this Mar 24, 2021

koraykoska pushed a commit that referenced this issue Apr 5, 2021

Fix #3425

357b89f

Added the custom error

koraykoska mentioned this issue Apr 5, 2021

Fix #3425 #3989

Closed

16 tasks

Polycarpik removed this from the Maintenance milestone May 12, 2021

github-actions bot added the Stale Has not received enough activity label Jul 12, 2021

github-actions bot removed the Stale Has not received enough activity label Jul 14, 2021

uluhonolulu added a commit to uluhonolulu/web3.js that referenced this issue Sep 5, 2021

Propagate the original request error (web3#3425)

b0dbe82

uluhonolulu added a commit to uluhonolulu/web3.js that referenced this issue Sep 5, 2021

Fixed a test (web3#3425)

fd4dcdc

spacesailor24 mentioned this issue Sep 8, 2021

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

Closed

github-actions bot added the Stale Has not received enough activity label Sep 12, 2021

spacesailor24 added In Progress Currently being worked on and removed Stale Has not received enough activity labels Sep 15, 2021

jdevcs linked a pull request Sep 16, 2021 that will close this issue

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

Closed

jdevcs mentioned this issue Oct 20, 2021

[Rewrite 4.x] Http Provider #4312

Closed

14 tasks

jdevcs closed this as completed Nov 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XMLHttpRequest errors are not handled correctly #3425

XMLHttpRequest errors are not handled correctly #3425

liamaharon commented Mar 22, 2020 •

edited

Loading

cgewecke commented Mar 24, 2020

wbt commented Apr 27, 2020 •

edited

Loading

github-actions bot commented Jul 3, 2020

wbt commented Jul 3, 2020

github-actions bot commented Aug 3, 2020

wbt commented Aug 3, 2020

uluhonolulu commented Sep 26, 2020

github-actions bot commented Nov 26, 2020

liamaharon commented Nov 26, 2020

uluhonolulu commented Nov 27, 2020

github-actions bot commented Jan 27, 2021

wbt commented Jan 27, 2021

uluhonolulu commented Mar 5, 2021

illuzen commented Apr 22, 2021

github-actions bot commented Jul 12, 2021

wbt commented Jul 13, 2021

uluhonolulu commented Jul 13, 2021

github-actions bot commented Sep 12, 2021

philknows commented Nov 2, 2021

zon-dtravel commented Apr 26, 2022

ghost commented Jul 5, 2022

XMLHttpRequest errors are not handled correctly #3425

XMLHttpRequest errors are not handled correctly #3425

Comments

liamaharon commented Mar 22, 2020 • edited Loading

cgewecke commented Mar 24, 2020

wbt commented Apr 27, 2020 • edited Loading

github-actions bot commented Jul 3, 2020

wbt commented Jul 3, 2020

github-actions bot commented Aug 3, 2020

wbt commented Aug 3, 2020

uluhonolulu commented Sep 26, 2020

github-actions bot commented Nov 26, 2020

liamaharon commented Nov 26, 2020

uluhonolulu commented Nov 27, 2020

github-actions bot commented Jan 27, 2021

wbt commented Jan 27, 2021

uluhonolulu commented Mar 5, 2021

illuzen commented Apr 22, 2021

github-actions bot commented Jul 12, 2021

wbt commented Jul 13, 2021

uluhonolulu commented Jul 13, 2021

github-actions bot commented Sep 12, 2021

philknows commented Nov 2, 2021

zon-dtravel commented Apr 26, 2022

ghost commented Jul 5, 2022

liamaharon commented Mar 22, 2020 •

edited

Loading

wbt commented Apr 27, 2020 •

edited

Loading