From 3e65be7b895d845cce4fb9712b83e356c01f3fc6 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Mon, 31 Jul 2023 17:32:49 +0200 Subject: [PATCH] refactor: switch to native fetch implementation (#5811) * Switch to native fetch implementation * Wrapper around native fetch to improve error handling * Improve handling of other native fetch errors * Update fetch error types * Native fetch errors might not have cause.code property * Check if instance of TypeError * Add unknown scheme error to examples * Add tests to detect changes native fetch error behavior * Remove unnecessary exports * Improve error.cause type * Move fetch to @lodestar/api package * Remove tsdoc from NativeFetchError * Add headers overflow test --- packages/api/package.json | 1 - packages/api/src/index.ts | 3 + packages/api/src/utils/client/fetch.ts | 137 ++++++++++++++++++ packages/api/src/utils/client/httpClient.ts | 6 +- packages/api/src/utils/client/index.ts | 1 + packages/api/test/unit/client/fetch.test.ts | 121 ++++++++++++++++ packages/api/test/utils/fetchOpenApiSpec.ts | 2 +- packages/beacon-node/package.json | 1 - .../src/eth1/provider/jsonRpcHttpClient.ts | 20 +-- .../beacon-node/src/execution/engine/utils.ts | 3 +- .../beacon-node/src/monitoring/service.ts | 2 +- .../test/e2e/api/impl/config.test.ts | 2 +- .../test/e2e/eth1/jsonRpcHttpClient.test.ts | 8 +- .../test/unit/metrics/server/http.test.ts | 2 +- .../test/unit/monitoring/service.test.ts | 2 +- packages/light-client/package.json | 1 - packages/validator/package.json | 1 - .../src/util/externalSignerClient.ts | 2 +- yarn.lock | 2 +- 19 files changed, 278 insertions(+), 39 deletions(-) create mode 100644 packages/api/src/utils/client/fetch.ts create mode 100644 packages/api/test/unit/client/fetch.test.ts diff --git a/packages/api/package.json b/packages/api/package.json index 3bd8e9d3c47e..3301dea27d7a 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -75,7 +75,6 @@ "@lodestar/params": "^1.9.2", "@lodestar/types": "^1.9.2", "@lodestar/utils": "^1.9.2", - "cross-fetch": "^3.1.8", "eventsource": "^2.0.2", "qs": "^6.11.1" }, diff --git a/packages/api/src/index.ts b/packages/api/src/index.ts index 5a8760f590a2..a0436611798e 100644 --- a/packages/api/src/index.ts +++ b/packages/api/src/index.ts @@ -10,6 +10,9 @@ export { HttpError, ApiError, Metrics, + FetchError, + isFetchError, + fetch, } from "./utils/client/index.js"; export * from "./utils/routes.js"; diff --git a/packages/api/src/utils/client/fetch.ts b/packages/api/src/utils/client/fetch.ts new file mode 100644 index 000000000000..9e98c61a91da --- /dev/null +++ b/packages/api/src/utils/client/fetch.ts @@ -0,0 +1,137 @@ +/** + * Native fetch with transparent and consistent error handling + * + * [MDN Reference](https://developer.mozilla.org/docs/Web/API/fetch) + */ +async function wrappedFetch(url: string | URL, init?: RequestInit): Promise { + try { + return await fetch(url, init); + } catch (e) { + throw new FetchError(url, e); + } +} + +export {wrappedFetch as fetch}; + +export function isFetchError(e: unknown): e is FetchError { + return e instanceof FetchError; +} + +export type FetchErrorType = "failed" | "input" | "aborted" | "unknown"; + +type FetchErrorCause = NativeFetchFailedError["cause"] | NativeFetchInputError["cause"]; + +export class FetchError extends Error { + type: FetchErrorType; + code: string; + cause?: FetchErrorCause; + + constructor(url: string | URL, e: unknown) { + if (isNativeFetchFailedError(e)) { + super(`Request to ${url.toString()} failed, reason: ${e.cause.message}`); + this.type = "failed"; + this.code = e.cause.code || "ERR_FETCH_FAILED"; + this.cause = e.cause; + } else if (isNativeFetchInputError(e)) { + // For input errors the e.message is more detailed + super(e.message); + this.type = "input"; + this.code = e.cause.code || "ERR_INVALID_INPUT"; + this.cause = e.cause; + } else if (isNativeFetchAbortError(e)) { + super(`Request to ${url.toString()} was aborted`); + this.type = "aborted"; + this.code = "ERR_ABORTED"; + } else { + super((e as Error).message); + this.type = "unknown"; + this.code = "ERR_UNKNOWN"; + } + this.name = this.constructor.name; + } +} + +type NativeFetchError = TypeError & { + cause: Error & { + code?: string; + }; +}; + +/** + * ``` + * TypeError: fetch failed + * cause: Error: connect ECONNREFUSED 127.0.0.1:9596 + * errno: -111, + * code: 'ECONNREFUSED', + * syscall: 'connect', + * address: '127.0.0.1', + * port: 9596 + * --------------------------- + * TypeError: fetch failed + * cause: Error: getaddrinfo ENOTFOUND non-existent-domain + * errno: -3008, + * code: 'ENOTFOUND', + * syscall: 'getaddrinfo', + * hostname: 'non-existent-domain' + * --------------------------- + * TypeError: fetch failed + * cause: SocketError: other side closed + * code: 'UND_ERR_SOCKET', + * socket: {} + * --------------------------- + * TypeError: fetch failed + * cause: Error: unknown scheme + * [cause]: undefined + * ``` + */ +type NativeFetchFailedError = NativeFetchError & { + message: "fetch failed"; + cause: { + errno?: string; + syscall?: string; + address?: string; + port?: string; + hostname?: string; + socket?: object; + [prop: string]: unknown; + }; +}; + +/** + * ``` + * TypeError: Failed to parse URL from invalid-url + * [cause]: TypeError [ERR_INVALID_URL]: Invalid URL + * input: 'invalid-url', + * code: 'ERR_INVALID_URL' + * ``` + */ +type NativeFetchInputError = NativeFetchError & { + cause: { + input: unknown; + }; +}; + +/** + * ``` + * DOMException [AbortError]: This operation was aborted + * ``` + */ +type NativeFetchAbortError = DOMException & { + name: "AbortError"; +}; + +function isNativeFetchError(e: unknown): e is NativeFetchError { + return e instanceof TypeError && (e as NativeFetchError).cause instanceof Error; +} + +function isNativeFetchFailedError(e: unknown): e is NativeFetchFailedError { + return isNativeFetchError(e) && (e as NativeFetchFailedError).message === "fetch failed"; +} + +function isNativeFetchInputError(e: unknown): e is NativeFetchInputError { + return isNativeFetchError(e) && (e as NativeFetchInputError).cause.input !== undefined; +} + +function isNativeFetchAbortError(e: unknown): e is NativeFetchAbortError { + return e instanceof DOMException && e.name === "AbortError"; +} diff --git a/packages/api/src/utils/client/httpClient.ts b/packages/api/src/utils/client/httpClient.ts index 084edaae2243..6015d9b6715f 100644 --- a/packages/api/src/utils/client/httpClient.ts +++ b/packages/api/src/utils/client/httpClient.ts @@ -1,7 +1,7 @@ -import {fetch} from "cross-fetch"; import {ErrorAborted, Logger, TimeoutError} from "@lodestar/utils"; import {ReqGeneric, RouteDef} from "../index.js"; import {ApiClientResponse, ApiClientSuccessResponse} from "../../interfaces.js"; +import {fetch, isFetchError} from "./fetch.js"; import {stringifyQuery, urlJoin} from "./format.js"; import {Metrics} from "./metrics.js"; import {HttpStatusCode} from "./httpStatusCode.js"; @@ -261,7 +261,7 @@ export class HttpClient implements IHttpClient { const timeout = setTimeout(() => controller.abort(), opts.timeoutMs ?? timeoutMs ?? this.globalTimeoutMs); // Attach global signal to this request's controller - const onGlobalSignalAbort = controller.abort.bind(controller); + const onGlobalSignalAbort = (): void => controller.abort(); const signalGlobal = this.getAbortSignal?.(); signalGlobal?.addEventListener("abort", onGlobalSignalAbort); @@ -323,7 +323,7 @@ export class HttpClient implements IHttpClient { } function isAbortedError(e: Error): boolean { - return e.name === "AbortError" || e.message === "The user aborted a request"; + return isFetchError(e) && e.type === "aborted"; } function getErrorMessage(errBody: string): string { diff --git a/packages/api/src/utils/client/index.ts b/packages/api/src/utils/client/index.ts index 6c0adec01c6b..7198c22ab89b 100644 --- a/packages/api/src/utils/client/index.ts +++ b/packages/api/src/utils/client/index.ts @@ -1,2 +1,3 @@ export * from "./client.js"; export * from "./httpClient.js"; +export * from "./fetch.js"; diff --git a/packages/api/test/unit/client/fetch.test.ts b/packages/api/test/unit/client/fetch.test.ts new file mode 100644 index 000000000000..bab1c57b2ab0 --- /dev/null +++ b/packages/api/test/unit/client/fetch.test.ts @@ -0,0 +1,121 @@ +import crypto from "node:crypto"; +import http from "node:http"; +import {expect} from "chai"; +import {FetchError, FetchErrorType, fetch} from "../../../src/utils/client/fetch.js"; + +describe("FetchError", function () { + const port = 37421; + const randomHex = crypto.randomBytes(32).toString("hex"); + + const testCases: { + id: string; + url?: string; + requestListener?: http.RequestListener; + abort?: true; + timeout?: number; + errorType: FetchErrorType; + errorCode: string; + expectCause: boolean; + }[] = [ + { + id: "Bad domain", + // Use random bytes to ensure no collisions + url: `https://${randomHex}.infura.io`, + errorType: "failed", + errorCode: "ENOTFOUND", + expectCause: true, + }, + { + id: "Bad port", + url: `http://localhost:${port + 1}`, + requestListener: (_req, res) => res.end(), + errorType: "failed", + errorCode: "ECONNREFUSED", + expectCause: true, + }, + { + id: "Socket error", + requestListener: (_req, res) => res.socket?.destroy(), + errorType: "failed", + errorCode: "UND_ERR_SOCKET", + expectCause: true, + }, + { + id: "Headers overflow", + requestListener: (_req, res) => { + res.setHeader("Large-Header", "a".repeat(1e6)); + res.end(); + }, + errorType: "failed", + errorCode: "UND_ERR_HEADERS_OVERFLOW", + expectCause: true, + }, + { + id: "Unknown scheme", + url: `httsp://localhost:${port}`, + errorType: "failed", + errorCode: "ERR_FETCH_FAILED", + expectCause: true, + }, + { + id: "Invalid URL", + url: "invalid-url", + errorType: "input", + errorCode: "ERR_INVALID_URL", + expectCause: true, + }, + { + id: "Aborted request", + abort: true, + requestListener: () => { + // leave the request open until aborted + }, + errorType: "aborted", + errorCode: "ERR_ABORTED", + expectCause: false, + }, + ]; + + const afterHooks: (() => Promise)[] = []; + + afterEach(async function () { + while (afterHooks.length) { + const afterHook = afterHooks.pop(); + if (afterHook) + await afterHook().catch((e: Error) => { + // eslint-disable-next-line no-console + console.error("Error in afterEach hook", e); + }); + } + }); + + for (const testCase of testCases) { + const {id, url = `http://localhost:${port}`, requestListener, abort} = testCase; + + it(id, async function () { + if (requestListener) { + const server = http.createServer(requestListener); + await new Promise((resolve) => server.listen(port, resolve)); + afterHooks.push( + () => + new Promise((resolve, reject) => + server.close((err) => { + if (err) reject(err); + else resolve(); + }) + ) + ); + } + + const controller = new AbortController(); + if (abort) setTimeout(() => controller.abort(), 20); + await expect(fetch(url, {signal: controller.signal})).to.be.rejected.then((error: FetchError) => { + expect(error.type).to.be.equal(testCase.errorType); + expect(error.code).to.be.equal(testCase.errorCode); + if (testCase.expectCause) { + expect(error.cause).to.be.instanceof(Error); + } + }); + }); + } +}); diff --git a/packages/api/test/utils/fetchOpenApiSpec.ts b/packages/api/test/utils/fetchOpenApiSpec.ts index 8db5bd809ec3..a4d7060fe55a 100644 --- a/packages/api/test/utils/fetchOpenApiSpec.ts +++ b/packages/api/test/utils/fetchOpenApiSpec.ts @@ -1,6 +1,6 @@ import fs from "node:fs"; import path from "node:path"; -import fetch from "cross-fetch"; +import {fetch} from "@lodestar/api"; import {OpenApiFile, OpenApiJson} from "./parseOpenApiSpec.js"; /* eslint-disable no-console */ diff --git a/packages/beacon-node/package.json b/packages/beacon-node/package.json index 96f37ce38366..c29d10f5ec91 100644 --- a/packages/beacon-node/package.json +++ b/packages/beacon-node/package.json @@ -136,7 +136,6 @@ "@types/datastore-level": "^3.0.0", "buffer-xor": "^2.0.2", "c-kzg": "^2.1.0", - "cross-fetch": "^3.1.8", "datastore-core": "^9.1.1", "datastore-level": "^10.1.1", "deepmerge": "^4.3.1", diff --git a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts index 999147236875..694848c14cf9 100644 --- a/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts +++ b/packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts @@ -1,7 +1,4 @@ -// Uses cross-fetch for browser + NodeJS cross compatibility -// Note: isomorphic-fetch is not well mantained and does not support abort signals -import fetch from "cross-fetch"; - +import {fetch} from "@lodestar/api"; import {ErrorAborted, TimeoutError, retry} from "@lodestar/utils"; import {IGauge, IHistogram} from "../../metrics/interface.js"; import {IJson, RpcPayload} from "../interface.js"; @@ -12,16 +9,6 @@ import {encodeJwtToken} from "./jwt.js"; const maxStringLengthToPrint = 500; const REQUEST_TIMEOUT = 30 * 1000; -// As we are using `cross-fetch` which does not support for types for errors -// We can't use `node-fetch` for browser compatibility -export type FetchError = { - errno: string; - code: string; -}; - -export const isFetchError = (error: unknown): error is FetchError => - (error as FetchError) !== undefined && "code" in (error as FetchError) && "errno" in (error as FetchError); - interface RpcResponse extends RpcResponseError { result?: R; } @@ -201,13 +188,8 @@ export class JsonRpcHttpClient implements IJsonRpcHttpClient { * Fetches JSON and throws detailed errors in case the HTTP request is not ok */ private async fetchJsonOneUrl(url: string, json: T, opts?: ReqOpts): Promise { - // If url is undefined node-fetch throws with `TypeError: Only absolute URLs are supported` - // Throw a better error instead if (!url) throw Error(`Empty or undefined JSON RPC HTTP client url: ${url}`); - // fetch() throws for network errors: - // - request to http://missing-url.com/ failed, reason: getaddrinfo ENOTFOUND missing-url.com - const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), opts?.timeout ?? this.opts?.timeout ?? REQUEST_TIMEOUT); diff --git a/packages/beacon-node/src/execution/engine/utils.ts b/packages/beacon-node/src/execution/engine/utils.ts index 1566c1d59984..3218d403baa6 100644 --- a/packages/beacon-node/src/execution/engine/utils.ts +++ b/packages/beacon-node/src/execution/engine/utils.ts @@ -1,5 +1,6 @@ +import {isFetchError} from "@lodestar/api"; import {IJson, RpcPayload} from "../../eth1/interface.js"; -import {IJsonRpcHttpClient, isFetchError} from "../../eth1/provider/jsonRpcHttpClient.js"; +import {IJsonRpcHttpClient} from "../../eth1/provider/jsonRpcHttpClient.js"; import {ExecutePayloadStatus, ExecutionEngineState} from "./interface.js"; export type JsonRpcBackend = { diff --git a/packages/beacon-node/src/monitoring/service.ts b/packages/beacon-node/src/monitoring/service.ts index 7f8f740e46f6..a6142aeac957 100644 --- a/packages/beacon-node/src/monitoring/service.ts +++ b/packages/beacon-node/src/monitoring/service.ts @@ -1,5 +1,5 @@ -import fetch from "cross-fetch"; import {Registry} from "prom-client"; +import {fetch} from "@lodestar/api"; import {ErrorAborted, Logger, TimeoutError} from "@lodestar/utils"; import {RegistryMetricCreator} from "../metrics/index.js"; import {HistogramExtra} from "../metrics/utils/histogram.js"; diff --git a/packages/beacon-node/test/e2e/api/impl/config.test.ts b/packages/beacon-node/test/e2e/api/impl/config.test.ts index 3d269733747c..e4fa5b29211e 100644 --- a/packages/beacon-node/test/e2e/api/impl/config.test.ts +++ b/packages/beacon-node/test/e2e/api/impl/config.test.ts @@ -1,4 +1,4 @@ -import fetch from "cross-fetch"; +import {fetch} from "@lodestar/api"; import {ForkName, activePreset} from "@lodestar/params"; import {chainConfig} from "@lodestar/config/default"; import {ethereumConsensusSpecsTests} from "../../../spec/specTestVersioning.js"; diff --git a/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts b/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts index 6de1cebf6fc8..7e68dc899fe6 100644 --- a/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts +++ b/packages/beacon-node/test/e2e/eth1/jsonRpcHttpClient.test.ts @@ -2,14 +2,11 @@ import "mocha"; import crypto from "node:crypto"; import http from "node:http"; import {expect} from "chai"; +import {FetchError} from "@lodestar/api"; import {JsonRpcHttpClient} from "../../../src/eth1/provider/jsonRpcHttpClient.js"; import {getGoerliRpcUrl} from "../../testParams.js"; import {RpcPayload} from "../../../src/eth1/interface.js"; -type FetchError = { - code: string; -}; - describe("eth1 / jsonRpcHttpClient", function () { this.timeout("10 seconds"); @@ -38,7 +35,8 @@ describe("eth1 / jsonRpcHttpClient", function () { id: "Bad subdomain", // Use random bytes to ensure no collisions url: `https://${randomHex}.infura.io`, - error: "getaddrinfo ENOTFOUND", + error: "", + errorCode: "ENOTFOUND", }, { id: "Bad port", diff --git a/packages/beacon-node/test/unit/metrics/server/http.test.ts b/packages/beacon-node/test/unit/metrics/server/http.test.ts index 330b099de76c..b147a283a960 100644 --- a/packages/beacon-node/test/unit/metrics/server/http.test.ts +++ b/packages/beacon-node/test/unit/metrics/server/http.test.ts @@ -1,4 +1,4 @@ -import fetch from "cross-fetch"; +import {fetch} from "@lodestar/api"; import {getHttpMetricsServer, HttpMetricsServer} from "../../../../src/metrics/index.js"; import {testLogger} from "../../../utils/logger.js"; import {createMetricsTest} from "../utils.js"; diff --git a/packages/beacon-node/test/unit/monitoring/service.test.ts b/packages/beacon-node/test/unit/monitoring/service.test.ts index 921d71a1b2d0..53ec4df355e8 100644 --- a/packages/beacon-node/test/unit/monitoring/service.test.ts +++ b/packages/beacon-node/test/unit/monitoring/service.test.ts @@ -195,7 +195,7 @@ describe("monitoring / service", () => { await service.send(); - assertError({message: `request to ${endpoint} failed, reason: connect ECONNREFUSED ${new URL(endpoint).host}`}); + assertError({message: `Request to ${endpoint} failed, reason: connect ECONNREFUSED ${new URL(endpoint).host}`}); }); it("should abort pending requests if timeout is reached", async () => { diff --git a/packages/light-client/package.json b/packages/light-client/package.json index 6be35f6d578f..ee6c8d519696 100644 --- a/packages/light-client/package.json +++ b/packages/light-client/package.json @@ -73,7 +73,6 @@ "@lodestar/state-transition": "^1.9.2", "@lodestar/types": "^1.9.2", "@lodestar/utils": "^1.9.2", - "cross-fetch": "^3.1.8", "mitt": "^3.0.0", "strict-event-emitter-types": "^2.0.0" }, diff --git a/packages/validator/package.json b/packages/validator/package.json index c199ceb2dbca..4c6ba5f43eb3 100644 --- a/packages/validator/package.json +++ b/packages/validator/package.json @@ -58,7 +58,6 @@ "@lodestar/types": "^1.9.2", "@lodestar/utils": "^1.9.2", "bigint-buffer": "^1.1.5", - "cross-fetch": "^3.1.8", "strict-event-emitter-types": "^2.0.0" }, "devDependencies": { diff --git a/packages/validator/src/util/externalSignerClient.ts b/packages/validator/src/util/externalSignerClient.ts index 4bcf66383b05..2716533e536f 100644 --- a/packages/validator/src/util/externalSignerClient.ts +++ b/packages/validator/src/util/externalSignerClient.ts @@ -1,5 +1,5 @@ -import fetch from "cross-fetch"; import {ContainerType, toHexString, ValueOf} from "@chainsafe/ssz"; +import {fetch} from "@lodestar/api"; import {phase0, altair, capella} from "@lodestar/types"; import {ForkSeq} from "@lodestar/params"; import {ValidatorRegistrationV1} from "@lodestar/types/bellatrix"; diff --git a/yarn.lock b/yarn.lock index 6bc904fe509d..cb4cbf41396c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6154,7 +6154,7 @@ create-require@^1.1.0: resolved "https://registry.npmjs.org/create-require/-/create-require-1.1.1.tgz" integrity sha512-dcKFX3jn0MpIaXjisoRvexIJVEKzaq7z2rZKxf+MSr9TkdmHmsU4m2lcLojrj/FHl8mk5VxMmYA+ftRkP/3oKQ== -cross-fetch@^3.1.4, cross-fetch@^3.1.8: +cross-fetch@^3.1.4: version "3.1.8" resolved "https://registry.yarnpkg.com/cross-fetch/-/cross-fetch-3.1.8.tgz#0327eba65fd68a7d119f8fb2bf9334a1a7956f82" integrity sha512-cvA+JwZoU0Xq+h6WkMvAUqPEYy92Obet6UdKLfW60qn99ftItKjB5T+BkyWOFWe2pUyfQ+IJHmpOTznqk1M6Kg==