Skip to content

Simplify HttpClient global AbortSignal handling #4108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dapplion wants to merge 2 commits into from
Closed

Simplify HttpClient global AbortSignal handling #4108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e75164
Fix HttpClient event handler leak
dapplion Jun 3, 2022
9bbf2a9
Merge branch 'unstable' into dapplion/http-client-global-signal
dapplion Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/api/src/client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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,
};
}
24 changes: 14 additions & 10 deletions packages/api/src/client/utils/httpClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,14 @@ export interface IHttpClient {
baseUrl: string;
json<T>(opts: FetchOpts): Promise<T>;
arrayBuffer(opts: FetchOpts): Promise<ArrayBuffer>;
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;
};
Expand All @@ -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;
Expand All @@ -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 {

@wemeetagain wemeetagain Jun 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this only be allowed once? Eg: if (this.signal) throw .. else this.signal = signal

@dapplion dapplion Jun 4, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, to allow the validator client to work it must allowed N times. Every time the validator is started a new signal is created, so it must be set to the HttpClient every time.

@wemeetagain wemeetagain Jun 5, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget that our validator can start and stop (and restart). I still think its weird.

this.signal = signal;
}

async json<T>(opts: FetchOpts): Promise<T> {
return await this.request<T>(opts, (res) => res.json() as Promise<T>);
}
Expand All @@ -81,9 +86,8 @@ export class HttpClient implements IHttpClient {

// Attach global signal to this request's controller
const onGlobalSignalAbort = controller.abort.bind(controller);
const signalGlobal = this.getAbortSignal?.();
if (signalGlobal) {
signalGlobal.addEventListener("abort", onGlobalSignalAbort);
if (this.signal) {
this.signal.addEventListener("abort", onGlobalSignalAbort);
}

const routeId = opts.routeId; // TODO: Should default to "unknown"?
Expand Down Expand Up @@ -114,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");
Expand All @@ -130,8 +134,8 @@ export class HttpClient implements IHttpClient {
timer?.();

clearTimeout(timeout);
if (signalGlobal) {
signalGlobal.removeEventListener("abort", onGlobalSignalAbort);
if (this.signal) {
this.signal.removeEventListener("abort", onGlobalSignalAbort);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/api/test/unit/client/httpClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
25 changes: 13 additions & 12 deletions packages/validator/src/validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}

/**
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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<typeof getClient> | Api): api is ReturnType<typeof getClient> {
return (api as ReturnType<typeof getClient>).httpClient !== undefined;
}