From 2e751648e18684d9b7fd9fa7f12728295281da00 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri, 3 Jun 2022 13:42:05 +0700 Subject: [PATCH] Fix HttpClient event handler leak --- packages/api/src/client/index.ts | 5 +++- packages/api/src/client/utils/httpClient.ts | 25 +++++++++++-------- .../api/test/unit/client/httpClient.test.ts | 2 +- packages/validator/src/validator.ts | 25 ++++++++++--------- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/packages/api/src/client/index.ts b/packages/api/src/client/index.ts index 7e3a48ea54da..f8c3cef00fe7 100644 --- a/packages/api/src/client/index.ts +++ b/packages/api/src/client/index.ts @@ -20,7 +20,7 @@ type ClientModules = HttpClientModules & { /** * REST HTTP client for all routes */ -export function getClient(opts: HttpClientOptions, modules: ClientModules): Api { +export function getClient(opts: HttpClientOptions, modules: ClientModules): Api & {httpClient: IHttpClient} { const {config} = modules; const httpClient = modules.httpClient ?? new HttpClient(opts, modules); @@ -33,5 +33,8 @@ export function getClient(opts: HttpClientOptions, modules: ClientModules): Api lodestar: lodestar.getClient(config, httpClient), node: node.getClient(config, httpClient), validator: validator.getClient(config, httpClient), + + // Extra for access to `IHttpClient.setAbortSignal` + httpClient, }; } diff --git a/packages/api/src/client/utils/httpClient.ts b/packages/api/src/client/utils/httpClient.ts index a312a0e865bc..3718286bf380 100644 --- a/packages/api/src/client/utils/httpClient.ts +++ b/packages/api/src/client/utils/httpClient.ts @@ -29,13 +29,14 @@ export interface IHttpClient { baseUrl: string; json(opts: FetchOpts): Promise; arrayBuffer(opts: FetchOpts): Promise; + setAbortSignal(signal: AbortSignal): void; } export type HttpClientOptions = { baseUrl: string; timeoutMs?: number; - /** Return an AbortSignal to be attached to all requests */ - getAbortSignal?: () => AbortSignal | undefined; + /** Global AbortSignal that cancels all active requests */ + signal?: AbortSignal; /** Override fetch function */ fetch?: typeof fetch; }; @@ -48,7 +49,7 @@ export type HttpClientModules = { export class HttpClient implements IHttpClient { readonly baseUrl: string; private readonly timeoutMs: number; - private readonly getAbortSignal?: () => AbortSignal | undefined; + private signal?: AbortSignal; private readonly fetch: typeof fetch; private readonly metrics: null | Metrics; private readonly logger: null | ILogger; @@ -60,12 +61,16 @@ export class HttpClient implements IHttpClient { this.baseUrl = opts.baseUrl; // A higher default timeout, validator will sets its own shorter timeoutMs this.timeoutMs = opts.timeoutMs ?? 60_000; - this.getAbortSignal = opts.getAbortSignal; + this.signal = opts.signal; this.fetch = opts.fetch ?? fetch; this.metrics = metrics ?? null; this.logger = logger ?? null; } + setAbortSignal(signal: AbortSignal): void { + this.signal = signal; + } + async json(opts: FetchOpts): Promise { return await this.request(opts, (res) => res.json() as Promise); } @@ -80,9 +85,9 @@ export class HttpClient implements IHttpClient { const timeout = setTimeout(() => controller.abort(), this.timeoutMs); // Attach global signal to this request's controller - const signalGlobal = this.getAbortSignal && this.getAbortSignal(); - if (signalGlobal) { - signalGlobal.addEventListener("abort", () => controller.abort()); + const onGlobalSignalAbort = controller.abort.bind(controller); + if (this.signal) { + this.signal.addEventListener("abort", onGlobalSignalAbort); } const routeId = opts.routeId; // TODO: Should default to "unknown"? @@ -113,7 +118,7 @@ export class HttpClient implements IHttpClient { return await getBody(res); } catch (e) { if (isAbortedError(e as Error)) { - if (signalGlobal?.aborted) { + if (this.signal?.aborted) { throw new ErrorAborted("REST client"); } else if (controller.signal.aborted) { throw new TimeoutError("request"); @@ -129,8 +134,8 @@ export class HttpClient implements IHttpClient { timer?.(); clearTimeout(timeout); - if (signalGlobal) { - signalGlobal.removeEventListener("abort", controller.abort); + if (this.signal) { + this.signal.removeEventListener("abort", onGlobalSignalAbort); } } } diff --git a/packages/api/test/unit/client/httpClient.test.ts b/packages/api/test/unit/client/httpClient.test.ts index 4ff890295101..9af5777a17ba 100644 --- a/packages/api/test/unit/client/httpClient.test.ts +++ b/packages/api/test/unit/client/httpClient.test.ts @@ -158,7 +158,7 @@ describe("httpClient json client", () => { const controller = new AbortController(); const signal = controller.signal; - const httpClient = new HttpClient({baseUrl, getAbortSignal: () => signal}); + const httpClient = new HttpClient({baseUrl, signal}); setTimeout(() => controller.abort(), 10); diff --git a/packages/validator/src/validator.ts b/packages/validator/src/validator.ts index 7b07beab7a63..426871677326 100644 --- a/packages/validator/src/validator.ts +++ b/packages/validator/src/validator.ts @@ -72,12 +72,9 @@ export class Validator { const api = typeof opts.api === "string" ? getClient( - { - baseUrl: opts.api, - // Validator would need the beacon to respond within the slot - timeoutMs: config.SECONDS_PER_SLOT * 1000, - getAbortSignal: this.getAbortSignal, - }, + // Validator would need the beacon to respond within the slot + // TODO: Allow to config timeoutMs via CLI args + {baseUrl: opts.api, timeoutMs: config.SECONDS_PER_SLOT * 1000}, {config, logger, metrics: metrics?.restApiClient} ) : opts.api; @@ -150,7 +147,7 @@ export class Validator { typeof opts.api === "string" ? // This new api instance can make do with default timeout as a faster timeout is // not necessary since this instance won't be used for validator duties - getClient({baseUrl: opts.api, getAbortSignal: () => signal}, {config, logger}) + getClient({baseUrl: opts.api, signal}, {config, logger}) : opts.api; const genesis = await waitForGenesis(api, opts.logger, signal); @@ -183,6 +180,10 @@ export class Validator { const {signal} = controller; this.clock.start(signal); this.chainHeaderTracker.start(signal); + + if (isApiHttp(this.api)) { + this.api.httpClient.setAbortSignal(signal); + } } /** @@ -214,11 +215,6 @@ export class Validator { this.logger.info(`Submitted voluntary exit for ${publicKey} to the network`); } - - /** Provide the current AbortSignal to the api instance */ - private getAbortSignal = (): AbortSignal | undefined => { - return this.state.status === Status.running ? this.state.controller.signal : undefined; - }; } /** Assert the same genesisValidatorRoot and genesisTime */ @@ -252,3 +248,8 @@ async function assertEqualGenesis(opts: ValidatorOptions, genesis: Genesis): Pro opts.logger.info("Persisted genesisTime", nodeGenesisTime); } } + +/** Type-guard to check if Api client is an HTTP client or else (in-memory client) */ +function isApiHttp(api: ReturnType | Api): api is ReturnType { + return (api as ReturnType).httpClient !== undefined; +}