fix(connector-besu): do not crash if ledger unreachable - send HTTP 503 #3573
Conversation
ruzell22
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
| diff --git a/lib/index.js b/lib/index.js | ||
| index de2853c247b334e6b22cee42b3e597281c44efdc..787aecf6758228c092f1a3cd8097f64e76cc573d 100644 | ||
| --- a/lib/index.js | ||
| +++ b/lib/index.js | ||
| @@ -344,7 +344,7 @@ function _handleTxPricing(_this, tx) { | ||
| throw Error("Network doesn't support eip-1559"); | ||
| resolve({ gasPrice }); | ||
| } | ||
| - }); | ||
| + }).catch((ex) => reject(ex)); | ||
| } | ||
| } | ||
| catch (error) { |
There was a problem hiding this comment.
Please provide more details about this (in commit & PR message). In particular:
- Link to an issue reported on web3js repository
- What this patch does
- Why we need this, what will happen if we remove it (specific scenario).
There was a problem hiding this comment.
@ruzell22 +1 to the above, this should be explained in detail.
@outSH I wrote the code, here's the answer that @ruzell22 can copy paste into the PR description and the commit message: Without the catch block the rejection is an unhandled rejection that bubbles up to the top of the callstack where NodeJS itself catches it and then crashes the entire process. (used to be that it just logged a warning but since some of the newer versions it crashes which allows us to find these bugs in our code / library's code)
There was a problem hiding this comment.
@petermetz this is a nice learning! (Never knew that we can patch and include packages this way as well)
There was a problem hiding this comment.
@jagpreetsinghsasan Yeah, it's very useful when you need it. :-)
There was a problem hiding this comment.
@ruzell22 Looking good, but please add a specific test case within the run-transaction endpoint's existing test suite so that we have a specific unit test verifying that this logic now works as intended.
Please put the test case in the test-plugin-ledger-connector-besu package because that way the API server can be used as well and so it's a more complete simulation.
Primary Changes --------------- 1. Yarn patch to web3-eth-acconts@1.6.1 so that it does not crash nodejs process 2. It is returning a 503 instead of a 500 3. Added a static retry-after header value of 5 seconds Without the catch block, the rejection is an unhandled rejection that bubbles up to the top of the callstack where NodeJS itself catches it and then crashes the entire process. (used to be that it just logged a warning but since some of the newer versions it crashes which allows us to find these bugs in our code / library's code) Fixes: hyperledger-cacti#3406 Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com> Signed-off-by: ruzell22 <ruzell.vince.aquino@accenture.com> Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
There was a problem hiding this comment.
@ruzell22 I can't find the relevant test case that creates the scenario in question (no ledger connectivity and then assertions to verify that the API server hasn't crashed and that the response was sent back as expected).
Commit to be reviewed
fix(connector-besu): do not crash if ledger unreachable - send HTTP 503
Fixes: #3406
Pull Request Requirements
upstream/mainbranch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-sflag when usinggit commitcommand. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.