refactor: update retry function (#6451)

* refactor: update retry function * chore: subtract 1 from retry values * Keep same number of retries in download test * Update retry function * Fix jsonRpcHttpClient tests * Restore comment --------- Co-authored-by: Nico Flaig <[email protected]>
ChainSafe · Feb 21, 2024 · 53f8f99 · 53f8f99
1 parent d1dc217
commit 53f8f99
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 18 deletions.
diff --git a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts
@@ -162,7 +162,7 @@ export class JsonRpcHttpClient implements IJsonRpcHttpClient {
           return this.fetchJson({jsonrpc: "2.0", id: this.id++, ...payload}, opts);
         },
         {
-          retries: opts?.retryAttempts ?? this.opts?.retryAttempts ?? 1,
+          retries: opts?.retryAttempts ?? this.opts?.retryAttempts ?? 0,
           retryDelay: opts?.retryDelay ?? this.opts?.retryDelay ?? 0,
           shouldRetry: opts?.shouldRetry,
           signal: this.opts?.signal,

diff --git a/packages/beacon-node/src/execution/builder/http.ts b/packages/beacon-node/src/execution/builder/http.ts
@@ -118,7 +118,7 @@ export class ExecutionBuilderHttp implements IExecutionBuilder {
   async submitBlindedBlock(
     signedBlindedBlock: allForks.SignedBlindedBeaconBlock
   ): Promise<allForks.SignedBeaconBlockOrContents> {
-    const res = await this.api.submitBlindedBlock(signedBlindedBlock, {retryAttempts: 3});
+    const res = await this.api.submitBlindedBlock(signedBlindedBlock, {retryAttempts: 2});
     ApiError.assert(res, "execution.builder.submitBlindedBlock");
     const {data} = res.response;
 

diff --git a/packages/beacon-node/src/execution/engine/http.ts b/packages/beacon-node/src/execution/engine/http.ts
@@ -72,7 +72,7 @@ export const defaultExecutionEngineHttpOpts: ExecutionEngineHttpOpts = {
    * port/url, one can override this and skip providing a jwt secret.
    */
   urls: ["http://localhost:8551"],
-  retryAttempts: 3,
+  retryAttempts: 2,
   retryDelay: 2000,
   timeout: 12000,
 };
@@ -305,7 +305,7 @@ export class ExecutionEngineHttp implements IExecutionEngine {
     // If we are just fcUing and not asking execution for payload, retry is not required
     // and we can move on, as the next fcU will be issued soon on the new slot
     const fcUReqOpts =
-      payloadAttributes !== undefined ? forkchoiceUpdatedV1Opts : {...forkchoiceUpdatedV1Opts, retryAttempts: 1};
+      payloadAttributes !== undefined ? forkchoiceUpdatedV1Opts : {...forkchoiceUpdatedV1Opts, retryAttempts: 0};
 
     const request = this.rpcFetchQueue.push({
       method,

diff --git a/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts b/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts
@@ -225,10 +225,10 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () {
   });
 
   it("should retry 404", async function () {
-    let retryCount = 0;
+    let requestCount = 0;
 
     const server = http.createServer((req, res) => {
-      retryCount++;
+      requestCount++;
       res.statusCode = 404;
       res.end();
     });
@@ -251,14 +251,14 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () {
     const controller = new AbortController();
     const eth1JsonRpcClient = new JsonRpcHttpClient([url], {signal: controller.signal});
     await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts})).rejects.toThrow("Not Found");
-    expect(retryCount).toBeWithMessage(retryAttempts, "404 responses should be retried before failing");
+    expect(requestCount).toBeWithMessage(retryAttempts + 1, "404 responses should be retried before failing");
   });
 
   it("should retry timeout", async function () {
-    let retryCount = 0;
+    let requestCount = 0;
 
     const server = http.createServer(async () => {
-      retryCount++;
+      requestCount++;
     });
 
     await new Promise<void>((resolve) => server.listen(port, resolve));
@@ -284,13 +284,13 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () {
     await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts, timeout})).rejects.toThrow(
       "Timeout request"
     );
-    expect(retryCount).toBeWithMessage(retryAttempts, "Timeout request should be retried before failing");
+    expect(requestCount).toBeWithMessage(retryAttempts + 1, "Timeout request should be retried before failing");
   });
 
   it("should retry aborted", async function () {
-    let retryCount = 0;
+    let requestCount = 0;
     const server = http.createServer(() => {
-      retryCount++;
+      requestCount++;
       // leave the request open until timeout
     });
 
@@ -314,14 +314,14 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () {
     setTimeout(() => controller.abort(), 50);
     const eth1JsonRpcClient = new JsonRpcHttpClient([url], {signal: controller.signal});
     await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts, timeout})).rejects.toThrow("Aborted");
-    expect(retryCount).toBeWithMessage(1, "Aborted request should be retried before failing");
+    expect(requestCount).toBeWithMessage(1, "Aborted request should be retried before failing");
   });
 
   it("should not retry payload error", async function () {
-    let retryCount = 0;
+    let requestCount = 0;
 
     const server = http.createServer((req, res) => {
-      retryCount++;
+      requestCount++;
       res.setHeader("Content-Type", "application/json");
       res.end(JSON.stringify({jsonrpc: "2.0", id: 83, error: noMethodError}));
     });
@@ -344,6 +344,6 @@ describe("eth1 / jsonRpcHttpClient - with retries", function () {
     const controller = new AbortController();
     const eth1JsonRpcClient = new JsonRpcHttpClient([url], {signal: controller.signal});
     await expect(eth1JsonRpcClient.fetchWithRetries(payload, {retryAttempts})).rejects.toThrow("Method not found");
-    expect(retryCount).toBeWithMessage(1, "Payload error (non-network error) should not be retried");
+    expect(requestCount).toBeWithMessage(1, "Payload error (non-network error) should not be retried");
   });
 }, {timeout: 10_000});
diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts
@@ -32,19 +32,25 @@ export type RetryOptions = {
  */
 export async function retry<A>(fn: (attempt: number) => A | Promise<A>, opts?: RetryOptions): Promise<A> {
   const maxRetries = opts?.retries ?? 5;
+  // Number of retries + the initial attempt
+  const maxAttempts = maxRetries + 1;
   const shouldRetry = opts?.shouldRetry;
   const onRetry = opts?.onRetry;
 
   let lastError: Error = Error("RetryError");
-  for (let i = 1; i <= maxRetries; i++) {
+  for (let i = 1; i <= maxAttempts; i++) {
     try {
       // If not the first attempt, invoke right before retrying
       if (i > 1) onRetry?.(lastError, i);
 
       return await fn(i);
     } catch (e) {
       lastError = e as Error;
-      if (shouldRetry && !shouldRetry(lastError)) {
+
+      if (i === maxAttempts) {
+        // Reached maximum number of attempts, there's no need to check if we should retry
+        break;
+      } else if (shouldRetry && !shouldRetry(lastError)) {
         break;
       } else if (opts?.retryDelay !== undefined) {
         await sleep(opts?.retryDelay, opts?.signal);