From 56bffdd510047fb0bb9369a2e82bc1aaad5c578d Mon Sep 17 00:00:00 2001 From: Dimi Kot Date: Fri, 12 Jan 2024 01:04:30 -0800 Subject: [PATCH] Expose used Agent instance in RestResponse; improve keep-alive timeouts logic (#1) Pull Request: https://github.com/clickup/rest-client/pull/1 (main) --- README.md | 2 +- docs/README.md | 2 +- docs/classes/RestResponse.md | 27 ++++++++++++++------ docs/interfaces/RestOptions.md | 41 +++++++++++++++---------------- docs/modules.md | 2 +- src/RestOptions.ts | 14 +++++------ src/RestRequest.ts | 37 +++++++++++----------------- src/RestResponse.ts | 2 ++ src/__tests__/RestRequest.test.ts | 12 ++++++++- src/__tests__/helpers.ts | 3 ++- src/helpers/depaginate.ts | 2 +- src/internal/RestFetchReader.ts | 19 ++++++++++++-- typedoc.json | 7 +++++- 13 files changed, 102 insertions(+), 68 deletions(-) diff --git a/README.md b/README.md index 1fe1858..50dd334 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# RestClient: a syntax sugar tool around Node fetch() API, tailored to work with typescript-is or superstruct validators +# rest-client: a syntax sugar tool around Node fetch() API, tailored to work with typescript-is or superstruct validators See also [Full API documentation](https://github.com/clickup/rest-client/blob/master/docs/modules.md). diff --git a/docs/README.md b/docs/README.md index a7d200b..ccceda5 100644 --- a/docs/README.md +++ b/docs/README.md @@ -1,6 +1,6 @@ @clickup/rest-client / [Exports](modules.md) -# RestClient: a syntax sugar tool around Node fetch() API, tailored to work with typescript-is or superstruct validators +# rest-client: a syntax sugar tool around Node fetch() API, tailored to work with typescript-is or superstruct validators See also [Full API documentation](https://github.com/clickup/rest-client/blob/master/docs/modules.md). diff --git a/docs/classes/RestResponse.md b/docs/classes/RestResponse.md index 37e1190..563d8d4 100644 --- a/docs/classes/RestResponse.md +++ b/docs/classes/RestResponse.md @@ -16,13 +16,14 @@ body and make it a part of RestResponse abstraction. ### constructor -• **new RestResponse**(`req`, `status`, `headers`, `text`, `textIsPartial`) +• **new RestResponse**(`req`, `agent`, `status`, `headers`, `text`, `textIsPartial`) #### Parameters | Name | Type | | :------ | :------ | | `req` | [`RestRequest`](RestRequest.md)<`any`\> | +| `agent` | ``null`` \| `Agent` | | `status` | `number` | | `headers` | `Headers` | | `text` | `string` | @@ -30,7 +31,7 @@ body and make it a part of RestResponse abstraction. #### Defined in -[src/RestResponse.ts:17](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L17) +[src/RestResponse.ts:18](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L18) ## Properties @@ -40,7 +41,17 @@ body and make it a part of RestResponse abstraction. #### Defined in -[src/RestResponse.ts:18](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L18) +[src/RestResponse.ts:19](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L19) + +___ + +### agent + +• `Readonly` **agent**: ``null`` \| `Agent` + +#### Defined in + +[src/RestResponse.ts:20](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L20) ___ @@ -50,7 +61,7 @@ ___ #### Defined in -[src/RestResponse.ts:19](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L19) +[src/RestResponse.ts:21](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L21) ___ @@ -60,7 +71,7 @@ ___ #### Defined in -[src/RestResponse.ts:20](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L20) +[src/RestResponse.ts:22](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L22) ___ @@ -70,7 +81,7 @@ ___ #### Defined in -[src/RestResponse.ts:21](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L21) +[src/RestResponse.ts:23](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L23) ___ @@ -80,7 +91,7 @@ ___ #### Defined in -[src/RestResponse.ts:22](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L22) +[src/RestResponse.ts:24](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L24) ## Accessors @@ -107,4 +118,4 @@ RestRequest.json() instead. #### Defined in -[src/RestResponse.ts:40](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L40) +[src/RestResponse.ts:42](https://github.com/clickup/rest-client/blob/master/src/RestResponse.ts#L42) diff --git a/docs/interfaces/RestOptions.md b/docs/interfaces/RestOptions.md index 84e0ab4..eb0ba53 100644 --- a/docs/interfaces/RestOptions.md +++ b/docs/interfaces/RestOptions.md @@ -16,7 +16,7 @@ web app, and we don't want to retry them. #### Defined in -[src/RestOptions.ts:69](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L69) +[src/RestOptions.ts:70](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L70) ___ @@ -28,7 +28,7 @@ How much time to wait by default on the 1st retry attempt. #### Defined in -[src/RestOptions.ts:71](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L71) +[src/RestOptions.ts:72](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L72) ___ @@ -40,7 +40,7 @@ How much to increase the retry delay on each retry. #### Defined in -[src/RestOptions.ts:73](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L73) +[src/RestOptions.ts:74](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L74) ___ @@ -53,7 +53,7 @@ Use this fraction (random) of the current retry delay to jitter both ways #### Defined in -[src/RestOptions.ts:76](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L76) +[src/RestOptions.ts:77](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L77) ___ @@ -65,7 +65,7 @@ Maximum delay between each retry. #### Defined in -[src/RestOptions.ts:78](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L78) +[src/RestOptions.ts:79](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L79) ___ @@ -84,7 +84,7 @@ A logic which runs on different IO stages (delay and heartbeats). #### Defined in -[src/RestOptions.ts:80](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L80) +[src/RestOptions.ts:81](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L81) ___ @@ -96,7 +96,7 @@ Allows to limit huge requests and throw instead. #### Defined in -[src/RestOptions.ts:91](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L91) +[src/RestOptions.ts:92](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L92) ___ @@ -109,7 +109,7 @@ response or not. #### Defined in -[src/RestOptions.ts:94](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L94) +[src/RestOptions.ts:95](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L95) ___ @@ -122,7 +122,7 @@ addresses are allowed. #### Defined in -[src/RestOptions.ts:97](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L97) +[src/RestOptions.ts:98](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L98) ___ @@ -134,7 +134,7 @@ If true, logs request-response pairs to console. #### Defined in -[src/RestOptions.ts:99](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L99) +[src/RestOptions.ts:100](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L100) ___ @@ -148,13 +148,12 @@ Sets Keep-Alive parameters (persistent connections). | Name | Type | Description | | :------ | :------ | :------ | -| `timeout` | `number` | A hint to the server, how much time to keep the connection alive. Not all the servers respect it though (e.g. nginx and express do not). | -| `max` | `number` | How many requests are allowed to be processed in one connection. | +| `timeoutMs` | `number` | How much time to keep an idle connection alive in the pool. If 0, closes the connection immediately after the response. | | `maxSockets?` | `number` | How many sockets at maximum will be kept open. | #### Defined in -[src/RestOptions.ts:103](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L103) +[src/RestOptions.ts:104](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L104) ___ @@ -166,7 +165,7 @@ When resolving DNS, use IPv4, IPv6 or both (see dns.lookup() docs). #### Defined in -[src/RestOptions.ts:113](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L113) +[src/RestOptions.ts:112](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L112) ___ @@ -178,7 +177,7 @@ Max timeout to wait for a response. #### Defined in -[src/RestOptions.ts:115](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L115) +[src/RestOptions.ts:114](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L114) ___ @@ -205,7 +204,7 @@ delay events logging. #### Defined in -[src/RestOptions.ts:118](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L118) +[src/RestOptions.ts:117](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L117) ___ @@ -217,7 +216,7 @@ Middlewares to wrap requests. May alter both request and response. #### Defined in -[src/RestOptions.ts:120](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L120) +[src/RestOptions.ts:119](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L119) ___ @@ -254,7 +253,7 @@ remote API is that weird. Return values: #### Defined in -[src/RestOptions.ts:134](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L134) +[src/RestOptions.ts:132](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L132) ___ @@ -288,7 +287,7 @@ contradictory information; then isRateLimitError wins. #### Defined in -[src/RestOptions.ts:144](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L144) +[src/RestOptions.ts:142](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L142) ___ @@ -315,7 +314,7 @@ not, the response ought to be either success or some other error. #### Defined in -[src/RestOptions.ts:149](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L149) +[src/RestOptions.ts:147](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L147) ___ @@ -348,4 +347,4 @@ retry will happen in not less than this number of milliseconds. #### Defined in -[src/RestOptions.ts:157](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L157) +[src/RestOptions.ts:155](https://github.com/clickup/rest-client/blob/master/src/RestOptions.ts#L155) diff --git a/docs/modules.md b/docs/modules.md index 2933093..581300f 100644 --- a/docs/modules.md +++ b/docs/modules.md @@ -53,7 +53,7 @@ On each call, the inner function needs to return an array with two elements: | Name | Type | | :------ | :------ | -| `readFunc` | (`cursor`: `undefined` \| `TCursor`) => `Promise`<[`TItem`[], `undefined` \| ``null`` \| `TCursor`]\> | +| `readFunc` | (`cursor`: `undefined` \| `TCursor`) => `Promise` | #### Returns diff --git a/src/RestOptions.ts b/src/RestOptions.ts index db7426e..7f3064b 100644 --- a/src/RestOptions.ts +++ b/src/RestOptions.ts @@ -37,6 +37,7 @@ export interface Middleware { */ interface AgentOptions { keepAlive: boolean; + timeout?: number; maxSockets?: number; rejectUnauthorized?: boolean; family?: 4 | 6 | 0; @@ -101,11 +102,9 @@ export default interface RestOptions { agents: Agents; /** Sets Keep-Alive parameters (persistent connections). */ keepAlive: { - /** A hint to the server, how much time to keep the connection alive. Not - * all the servers respect it though (e.g. nginx and express do not). */ - timeout: number; - /** How many requests are allowed to be processed in one connection. */ - max: number; + /** How much time to keep an idle connection alive in the pool. If 0, closes + * the connection immediately after the response. */ + timeoutMs: number; /** How many sockets at maximum will be kept open. */ maxSockets?: number; }; @@ -129,8 +128,7 @@ export default interface RestOptions { * performed; * * "THROW" - the request resulted in error. Additional tests will be * performed to determine is the error is retriable, is OAuth token good, - * and etc. - */ + * and etc. */ isSuccessResponse: (res: RestResponse) => "SUCCESS" | "THROW" | "BEST_EFFORT"; /** Decides whether the response is a rate-limit error or not. Returning * non-zero value is treated as retry delay (if retries are set up). In case @@ -176,7 +174,7 @@ export const DEFAULT_OPTIONS: RestOptions = { allowInternalIPs: false, isDebug: false, agents: new Agents(), - keepAlive: { timeout: 10, max: 100 }, + keepAlive: { timeoutMs: 10000 }, family: 4, // we don't want to hit ~5s DNS timeout on a missing IPv6 by default timeoutMs: 4 * 60 * 1000, logger: () => {}, diff --git a/src/RestRequest.ts b/src/RestRequest.ts index 06687c3..d23bac0 100644 --- a/src/RestRequest.ts +++ b/src/RestRequest.ts @@ -158,9 +158,9 @@ export default class RestRequest { reader = req._createFetchReader(fetchReq); await reader.preload(preloadChars); } finally { - res = - req._createRestResponse(reader) ?? - new RestResponse(req, 0, new Headers(), "", false); + res = reader + ? req._createRestResponse(reader) + : new RestResponse(req, null, 0, new Headers(), "", false); } throwIfErrorResponse(req.options, res); @@ -293,16 +293,9 @@ export default class RestRequest { redirectMode = "error"; } - const hasKeepAlive = this.options.keepAlive.timeout > 0; + const hasKeepAlive = this.options.keepAlive.timeoutMs > 0; if (hasKeepAlive) { headers.append("connection", "Keep-Alive"); - headers.append( - "keep-alive", - "timeout=" + - this.options.keepAlive.timeout + - ", max=" + - this.options.keepAlive.max - ); } // Use lazily created/cached per-RestClient Agent instance to utilize HTTP @@ -313,7 +306,8 @@ export default class RestRequest { : this.options.agents.http.bind(this.options.agents) )({ keepAlive: hasKeepAlive, - maxSockets: this.options.keepAlive?.maxSockets, + timeout: hasKeepAlive ? this.options.keepAlive.timeoutMs : undefined, + maxSockets: this.options.keepAlive.maxSockets, rejectUnauthorized: this.options.allowInternalIPs ? false : undefined, family: this.options.family, }); @@ -360,16 +354,15 @@ export default class RestRequest { * Creates a RestResponse from a RestFetchReader. Assumes that * RestFetchReader.preload() has already been called. */ - private _createRestResponse(reader: RestFetchReader | null) { - return reader - ? new RestResponse( - this, - reader.status, - reader.headers, - reader.textFetched, - reader.textIsPartial - ) - : null; + private _createRestResponse(reader: RestFetchReader) { + return new RestResponse( + this, + reader.agent, + reader.status, + reader.headers, + reader.textFetched, + reader.textIsPartial + ); } /** diff --git a/src/RestResponse.ts b/src/RestResponse.ts index b18ce03..bfef4c9 100644 --- a/src/RestResponse.ts +++ b/src/RestResponse.ts @@ -1,3 +1,4 @@ +import type { Agent as HttpAgent } from "http"; import { Memoize } from "fast-typescript-memoize"; import type { Headers } from "node-fetch"; import type RestRequest from "./RestRequest"; @@ -16,6 +17,7 @@ import type RestRequest from "./RestRequest"; export default class RestResponse { constructor( public readonly req: RestRequest, + public readonly agent: HttpAgent | null, public readonly status: number, public readonly headers: Headers, public readonly text: string, diff --git a/src/__tests__/RestRequest.test.ts b/src/__tests__/RestRequest.test.ts index a234f82..8b47012 100644 --- a/src/__tests__/RestRequest.test.ts +++ b/src/__tests__/RestRequest.test.ts @@ -13,14 +13,23 @@ import RestResponse from "../RestResponse"; const REQUEST = new RestClient().get("https://example.com"); const SUCCESS_RESPONSE = new RestResponse( REQUEST, + null, 200, new Headers(), "", false ); -const ERROR_RESPONSE = new RestResponse(REQUEST, 500, new Headers(), "", false); +const ERROR_RESPONSE = new RestResponse( + REQUEST, + null, + 500, + new Headers(), + "", + false +); const NOT_FOUND_RESPONSE = new RestResponse( REQUEST, + null, 404, new Headers(), "", @@ -28,6 +37,7 @@ const NOT_FOUND_RESPONSE = new RestResponse( ); const TOO_MANY_REQUESTS_RESPONSE = new RestResponse( REQUEST, + null, 429, new Headers(), "", diff --git a/src/__tests__/helpers.ts b/src/__tests__/helpers.ts index 38b267b..5e08c38 100644 --- a/src/__tests__/helpers.ts +++ b/src/__tests__/helpers.ts @@ -88,7 +88,7 @@ export const server = http.createServer(async (req, res) => { export async function serverAssertConnectionsCount(expect: number) { let got: number = 0; - for (let timeStart = Date.now(); Date.now() - timeStart < 2000; ) { + for (let timeStart = Date.now(); Date.now() - timeStart < 10000; ) { got = await new Promise((resolve) => server.getConnections((_, count) => resolve(count)) ); @@ -129,6 +129,7 @@ export async function createRestStream(url: string, preloadBytes?: number) { return new RestStream( new RestResponse( new RestRequest(DEFAULT_OPTIONS, "GET", "dummy", new Headers(), ""), + reader.agent, reader.status, reader.headers, reader.textFetched.toString(), diff --git a/src/helpers/depaginate.ts b/src/helpers/depaginate.ts index 604be61..ca5d104 100644 --- a/src/helpers/depaginate.ts +++ b/src/helpers/depaginate.ts @@ -9,7 +9,7 @@ export default async function* depaginate( readFunc: ( cursor: TCursor | undefined - ) => Promise<[TItem[], TCursor | null | undefined]> + ) => Promise ): AsyncGenerator { let prevCursor: TCursor | null | undefined = undefined; let cursor: TCursor | null | undefined = undefined; diff --git a/src/internal/RestFetchReader.ts b/src/internal/RestFetchReader.ts index 686decd..3654ab2 100644 --- a/src/internal/RestFetchReader.ts +++ b/src/internal/RestFetchReader.ts @@ -1,3 +1,4 @@ +import type { Agent as HttpAgent } from "http"; import AbortControllerPolyfilled from "abort-controller"; import { Memoize } from "fast-typescript-memoize"; import type { RequestInit } from "node-fetch"; @@ -32,7 +33,7 @@ export default class RestFetchReader { constructor( private _url: string, - private _req: RequestInit, + private _reqInit: RequestInit, private _options: RestFetchReaderOptions ) {} @@ -43,6 +44,20 @@ export default class RestFetchReader { return this._charsRead; } + /** + * Returns the Agent instance used for this request. It's implied that + * RestRequest#agent always points to a http.Agent object. + */ + get agent() { + return ( + this._reqInit.agent && + typeof this._reqInit.agent === "object" && + "sockets" in this._reqInit.agent + ? this._reqInit.agent + : null + ) as HttpAgent | null; + } + /** * Returns HTTP status after preload() was called. */ @@ -134,7 +149,7 @@ export default class RestFetchReader { const res = await fetch( this._url, new Request(this._url, { - ...this._req, + ...this._reqInit, signal: controller.signal as any, }) ); diff --git a/typedoc.json b/typedoc.json index 6dea761..d568b9e 100644 --- a/typedoc.json +++ b/typedoc.json @@ -1,6 +1,11 @@ { "entryPoints": ["src"], - "exclude": ["src/internal/**", "src/__tests__/**", "**/node-fetch/**", "**/node_modules/**"], + "exclude": [ + "**/internal/**", + "**/__tests__/**", + "**/node_modules/**", + "**/node-fetch/**" + ], "entryPointStrategy": "expand", "mergeModulesMergeMode": "project", "sort": ["source-order"],