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.

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

Allow JsonRpcProvider to retry getting its network after instantiation #814

Closed
jarindr opened this issue May 3, 2020 · 12 comments
Closed
Labels
enhancement New feature or improvement.

Comments

@jarindr
Copy link

jarindr commented May 3, 2020

initializing jsonRpcProvider and calling any function before geth is healthy/run will return
could not detect network (code=NETWORK_ERROR, version=providers/5.0.0-beta.164)
this seems to work as expected

but once the geth is backed up, the error still persists.

I tried changing all the call to create a new instance of jsonRpcProvider and call the functions instead of initializing once as a singleton as example here

From

const ethers = require('ethers')
const { ETH_JSON_RPC_URL } = require('../../config')
const jsonRpcProvider = new ethers.providers.JsonRpcProvider({
  url: ETH_JSON_RPC_URL
})
module.exports = jsonRpcProvider

To

const ethers = require('ethers')
const { ETH_JSON_RPC_URL } = require('../../config')
const jsonRpcProvider = () => new ethers.providers.JsonRpcProvider({
  url: ETH_JSON_RPC_URL
})
module.exports = jsonRpcProvider

Now it works, once geth is up the server can connect to it without any problem.

also, if the jsonRpc is created after geth is healthy, but geth dies after the error is different

TypeError: Cannot read property 'body' of undefined
    at /home/jarindr/works/bitpend-backend/node_modules/@ethersproject/web/lib/index.js:138:41
    at step (/home/jarindr/works/bitpend-backend/node_modules/@ethersproject/web/lib/index.js:33:23)
    at Object.throw (/home/jarindr/works/bitpend-backend/node_modules/@ethersproject/web/lib/index.js:14:53)
    at rejected (/home/jarindr/works/bitpend-backend/node_modules/@ethersproject/web/lib/index.js:6:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

note: this also happen with v4 with different error like invalid response 0

@jarindr jarindr changed the title [v5] initializing jsonRpcProvider before geth is accessible make all the calls faild [v5] initializing jsonRpcProvider before geth is accessible make all the calls failed May 3, 2020
@ricmoo ricmoo added investigate Under investigation and may be a bug. on-deck This Enhancement or Bug is currently being worked on. labels May 3, 2020
@ricmoo
Copy link
Member

ricmoo commented May 3, 2020

During that time, can you hit the Geth externally, like using curl?

@jarindr
Copy link
Author

jarindr commented May 3, 2020

@ricmoo yes, I have a service that check geth health and it is healthy once it is backup and all the jsonRPC works.

The Cannot read property 'body' of undefined problem probably come from
.on('block')event subscription
seems it is getting an error from eth node when it couldn't talk to it and didn't parse the error correctly.

@jarindr jarindr closed this as completed May 3, 2020
@jarindr jarindr reopened this May 3, 2020
@ricmoo
Copy link
Member

ricmoo commented May 3, 2020

Yeah, I should trap the bad body results (it is returning XML, probably) and throw a server error instead.

If you connect a JsonRpcProvider to a node that is down, and do not provide a network, it will currently never work, but that is something I could easily change.

Until the feature is added, you can resolve this by passing in a network as the second parameter. But I’ll look into this today. I have a few other things I want to get to today too and this lines up well with them. :)

@ricmoo ricmoo added enhancement New feature or improvement. next version (v5) and removed investigate Under investigation and may be a bug. labels May 3, 2020
@ricmoo ricmoo changed the title [v5] initializing jsonRpcProvider before geth is accessible make all the calls failed Allow JsonRpcProvider to retry getting its network after instantiation May 3, 2020
@jarindr
Copy link
Author

jarindr commented May 3, 2020

@ricmoo passing a networks works ! thank you. Since i'm running a geth POA just for e2e test, reading from the doc what kind of network name I should specify? kinda get that it doesn't matter but it would be good to get it documented.

Thank you for creating such a well-written eth library!

@ricmoo
Copy link
Member

ricmoo commented May 3, 2020

Oh, you should be able to pass in just the chain ID for PoA (as a number). Not sure what that is though, but you might know it or it is prolly easy to look up on your node.

You can pass in a string (like “homestead” or “ropsten”), a number (like 3 or 1337) or an object, if you need to configure ENS.

@jarindr
Copy link
Author

jarindr commented May 3, 2020

okay ! thanks for the help.

@ricmoo
Copy link
Member

ricmoo commented May 7, 2020

This should work now in 5.0.0-beta.185. The Provider API was tweaked a bit to better support network discovery in the event it fails when created.

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. fixed/complete This Bug is fixed or Enhancement is complete and published. labels May 7, 2020
@jarindr
Copy link
Author

jarindr commented May 8, 2020

@ricmoo will try it out !

@danielattilasimon
Copy link

This fix broke JsonRpcProvider for me :(

Added a comment to the offending line:
99ae946#r39033692

danielattilasimon added a commit to liquity/dev that referenced this issue May 8, 2020
@ricmoo
Copy link
Member

ricmoo commented May 9, 2020

Can you try 5.0.0-beta.186 that was release this afternoon and let me know if you still have any problems?

danielattilasimon added a commit to liquity/liquity that referenced this issue May 13, 2020
@ricmoo
Copy link
Member

ricmoo commented May 13, 2020

Closing this now as the above comment link indicates the latest version has fixed the issue. If not, please re-open.

Thanks! :)

@ricmoo ricmoo closed this as completed May 13, 2020
@ricmoo
Copy link
Member

ricmoo commented May 13, 2020

(Also, the above issue is referenced in #822)

michaeltout pushed a commit to michaeltout/ethers.js that referenced this issue Aug 23, 2020
danielattilasimon added a commit to liquity/liquity that referenced this issue Dec 16, 2020
danielattilasimon added a commit to liquity/liquity that referenced this issue Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement.
Projects
None yet
Development

No branches or pull requests

3 participants