Skip to content

feat: add retry logic to external signer - #2004

Merged
DariusParvin merged 1 commit into
masterfrom
BG-43083-external-signer-retry
Feb 17, 2022
Merged

feat: add retry logic to external signer#2004
DariusParvin merged 1 commit into
masterfrom
BG-43083-external-signer-retry

Conversation

@DariusParvin

Copy link
Copy Markdown
Contributor

if the request to the external signer fails with ECONNREFUSED, retry the
request three times with exponentially increasing timeout up to 3 times.

Ticket: BG-43083

@DariusParvin
DariusParvin force-pushed the BG-43083-external-signer-retry branch 2 times, most recently from fa008ba to 8eb1490 Compare February 15, 2022 18:10
@codecov

codecov Bot commented Feb 15, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2004 (32d2439) into master (3e2056c) will increase coverage by 52.44%.
The diff coverage is 14.28%.

❗ Current head 32d2439 differs from pull request most recent head 05e198a. Consider uploading reports for the commit 05e198a to get more accurate results

@@             Coverage Diff             @@
##           master    #2004       +/-   ##
===========================================
+ Coverage        0   52.44%   +52.44%     
===========================================
  Files           0        6        +6     
  Lines           0      553      +553     
  Branches        0       85       +85     
===========================================
+ Hits            0      290      +290     
- Misses          0      258      +258     
- Partials        0        5        +5     
Flag Coverage Δ
unit 52.44% <14.28%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
modules/express/src/retryPromise.ts 11.76% <11.76%> (ø)
modules/express/src/clientRoutes.ts 32.16% <25.00%> (ø)
modules/express/src/config.ts 100.00% <0.00%> (ø)
modules/express/src/args.ts 100.00% <0.00%> (ø)
modules/express/src/errors.ts 83.33% <0.00%> (ø)
modules/express/src/expressApp.ts 89.84% <0.00%> (ø)

@DariusParvin
DariusParvin marked this pull request as ready for review February 15, 2022 18:19
Comment thread modules/express/src/retryPromise.ts Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

when extending Error you must set the prototype otherwise it will break the prototype chain
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html#support-for-newtarget

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @lifeiscontent! Is this just a matter of adding the following line under super()?

Object.setPrototypeOf(this, new.target.prototype);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lifeiscontent that fix is no longer needed since we switched to targeting ES2019

@DariusParvin
DariusParvin force-pushed the BG-43083-external-signer-retry branch 4 times, most recently from d46491b to 26e9164 Compare February 16, 2022 02:33
if the request to the external signer fails with ECONNREFUSED, retry the
request three times with exponentially increasing timeout up to 3 times.

Ticket: BG-43083
@DariusParvin
DariusParvin force-pushed the BG-43083-external-signer-retry branch from 26e9164 to 05e198a Compare February 17, 2022 20:13

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

small nit, feel free to handle it in a follow up PR

.type('json')
.send({ txPrebuild: params.txPrebuild, pubs: params.pubs }),
(err, tryCount) => {
console.error(`attempt number ${tryCount}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's make this a debug instead and make it a little more informative

Suggested change
console.error(`attempt number ${tryCount}`);
debug(`failed to connect to external signer (attempt ${tryCount}, error: ${err.message})`);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added to #1989

@DariusParvin
DariusParvin merged commit fa84b50 into master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants