Skip to content

Commit

Permalink
refactor: switch to native fetch implementation (#5811)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
nflaig authored Jul 31, 2023
1 parent 3b37e28 commit 3e65be7
Show file tree
Hide file tree
Showing 19 changed files with 278 additions and 39 deletions.
1 change: 0 additions & 1 deletion packages/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export {
HttpError,
ApiError,
Metrics,
FetchError,
isFetchError,
fetch,
} from "./utils/client/index.js";
export * from "./utils/routes.js";

Expand Down
137 changes: 137 additions & 0 deletions packages/api/src/utils/client/fetch.ts
Original file line number Diff line number Diff line change
@@ -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<Response> {
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";
}
6 changes: 3 additions & 3 deletions packages/api/src/utils/client/httpClient.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions packages/api/src/utils/client/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from "./client.js";
export * from "./httpClient.js";
export * from "./fetch.js";
121 changes: 121 additions & 0 deletions packages/api/test/unit/client/fetch.test.ts
Original file line number Diff line number Diff line change
@@ -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<void>)[] = [];

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<void>((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);
}
});
});
}
});
2 changes: 1 addition & 1 deletion packages/api/test/utils/fetchOpenApiSpec.ts
Original file line number Diff line number Diff line change
@@ -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 */
Expand Down
1 change: 0 additions & 1 deletion packages/beacon-node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
20 changes: 1 addition & 19 deletions packages/beacon-node/src/eth1/provider/jsonRpcHttpClient.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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<R> extends RpcResponseError {
result?: R;
}
Expand Down Expand Up @@ -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<R, T = unknown>(url: string, json: T, opts?: ReqOpts): Promise<R> {
// 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);

Expand Down
3 changes: 2 additions & 1 deletion packages/beacon-node/src/execution/engine/utils.ts
Original file line number Diff line number Diff line change
@@ -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 = {
Expand Down
2 changes: 1 addition & 1 deletion packages/beacon-node/src/monitoring/service.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
2 changes: 1 addition & 1 deletion packages/beacon-node/test/e2e/api/impl/config.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
Loading

0 comments on commit 3e65be7

Please sign in to comment.