From 3d817c8abda2b108b426b8da2bb4d070b17066f4 Mon Sep 17 00:00:00 2001 From: Faybian Byrd Date: Sun, 18 Feb 2024 20:03:43 -0500 Subject: [PATCH 1/6] refactor: update retry function --- packages/utils/src/retry.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts index 19ebffc16898..c81ba5acad32 100644 --- a/packages/utils/src/retry.ts +++ b/packages/utils/src/retry.ts @@ -26,10 +26,12 @@ export type RetryOptions = { */ export async function retry(fn: (attempt: number) => A | Promise, opts?: RetryOptions): Promise { const maxRetries = opts?.retries ?? 5; + // The maximum number of attempts is the number of retries + the initial attempt + const maxAttempts = maxRetries + 1; const shouldRetry = opts?.shouldRetry; let lastError: Error = Error("RetryError"); - for (let i = 1; i <= maxRetries; i++) { + for (let i = 1; i <= maxAttempts; i++) { try { return await fn(i); } catch (e) { From 9ed842b8a91b8c0be89e2744bb907f28e76406e8 Mon Sep 17 00:00:00 2001 From: Faybian Byrd Date: Tue, 20 Feb 2024 17:28:49 -0500 Subject: [PATCH 2/6] chore: subtract 1 from retry values --- packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts | 2 +- packages/beacon-node/src/execution/builder/http.ts | 2 +- packages/beacon-node/src/execution/engine/http.ts | 4 ++-- packages/spec-test-util/src/downloadTests.ts | 2 +- packages/utils/src/retry.ts | 6 +++++- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts index 17809e047944..93ad34a33972 100644 --- a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts +++ b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts @@ -166,7 +166,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 index b4013c384f81..ca37bfa4072c 100644 --- 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 { - 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 index 91ceabaf2770..b609defb442c 100644 --- 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/spec-test-util/src/downloadTests.ts b/packages/spec-test-util/src/downloadTests.ts index 9e90d5825bdf..0e2002367e49 100644 --- a/packages/spec-test-util/src/downloadTests.ts +++ b/packages/spec-test-util/src/downloadTests.ts @@ -85,7 +85,7 @@ export async function downloadGenericSpecTests( log(`Downloaded ${url}`); }, { - retries: 3, + retries: 2, onRetry: (e, attempt) => { log(`Download attempt ${attempt} for ${url} failed: ${e.message}`); }, diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts index c81ba5acad32..64e14cc97b94 100644 --- a/packages/utils/src/retry.ts +++ b/packages/utils/src/retry.ts @@ -36,7 +36,11 @@ export async function retry(fn: (attempt: number) => A | Promise, opts?: R return await fn(i); } catch (e) { lastError = e as Error; - if (shouldRetry && !shouldRetry(lastError)) { + + if (i == maxAttempts) { + // If we have reached the 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); From 1bc3da92ec2e446d4b5c27e247eb9da19ca45be2 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Wed, 21 Feb 2024 11:49:33 +0100 Subject: [PATCH 3/6] Keep same number of retries in download test --- packages/spec-test-util/src/downloadTests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/spec-test-util/src/downloadTests.ts b/packages/spec-test-util/src/downloadTests.ts index 0e2002367e49..9e90d5825bdf 100644 --- a/packages/spec-test-util/src/downloadTests.ts +++ b/packages/spec-test-util/src/downloadTests.ts @@ -85,7 +85,7 @@ export async function downloadGenericSpecTests( log(`Downloaded ${url}`); }, { - retries: 2, + retries: 3, onRetry: (e, attempt) => { log(`Download attempt ${attempt} for ${url} failed: ${e.message}`); }, From c5491cbcda42879dcf1f4a124e717d753cf7c724 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Wed, 21 Feb 2024 11:50:09 +0100 Subject: [PATCH 4/6] Update retry function --- packages/utils/src/retry.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts index 64e14cc97b94..c874d41948d8 100644 --- a/packages/utils/src/retry.ts +++ b/packages/utils/src/retry.ts @@ -26,7 +26,6 @@ export type RetryOptions = { */ export async function retry(fn: (attempt: number) => A | Promise, opts?: RetryOptions): Promise { const maxRetries = opts?.retries ?? 5; - // The maximum number of attempts is the number of retries + the initial attempt const maxAttempts = maxRetries + 1; const shouldRetry = opts?.shouldRetry; @@ -37,8 +36,8 @@ export async function retry(fn: (attempt: number) => A | Promise, opts?: R } catch (e) { lastError = e as Error; - if (i == maxAttempts) { - // If we have reached the maximum number of attempts, there's no need to check if we should retry + 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; From 0dbfa66504c5fd2d6e83230ee36e5ac9d66c2df3 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Wed, 21 Feb 2024 11:50:34 +0100 Subject: [PATCH 5/6] Fix jsonRpcHttpClient tests --- .../test/e2e/eth1/jsonRpcHttpClient.test.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts b/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts index cf6d769ed9a3..277073bf10a8 100644 --- 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((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}); From ca7dfde62ddf561ae695cc35b79fa79cdeaba06b Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Wed, 21 Feb 2024 12:00:22 +0100 Subject: [PATCH 6/6] Restore comment --- packages/utils/src/retry.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/utils/src/retry.ts b/packages/utils/src/retry.ts index c5edbb9c8799..d875e81c75d9 100644 --- a/packages/utils/src/retry.ts +++ b/packages/utils/src/retry.ts @@ -32,6 +32,7 @@ export type RetryOptions = { */ export async function retry(fn: (attempt: number) => A | Promise, opts?: RetryOptions): Promise { const maxRetries = opts?.retries ?? 5; + // Number of retries + the initial attempt const maxAttempts = maxRetries + 1; const shouldRetry = opts?.shouldRetry; const onRetry = opts?.onRetry;