Add optional timeout parameter to HTTP client #111
Conversation
This change allows nix-installer-action etc. to avoid hard-coding a 10-second timeout for fetching Nix binaries.
WalkthroughIntroduces an optional timeout parameter across HTTP client creation and artifact download flows. The timeout propagates from public APIs through internal methods to ids-host’s Got client construction, overriding the default request timeout when provided. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant Index as index.ts (Public API)
participant Host as ids-host.ts (getGot)
participant Got as got client
App->>Index: fetchExecutable(timeout?)
activate Index
Index->>Index: fetchArtifact(timeout?)
Index->>Index: getClient(timeout?)
Index->>Host: getGot(callback?, timeout?)
activate Host
Host->>Got: got.extend({ timeout: timeout or DEFAULT_TIMEOUT })
deactivate Host
Index->>Got: HTTP requests (download/version) with client
deactivate Index
Note over App,Got: If timeout provided, it overrides default during client initialization.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for detsys-ts-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
737-809: Add optional timeout parameter to unpackClosure and pass it to fetchArtifact
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dist/index.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/ids-host.ts(1 hunks)src/index.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/ids-host.ts (1)
dist/index.js (1)
DEFAULT_TIMEOUT(412-412)
src/index.ts (1)
dist/index.js (4)
binaryPath(914-914)versionCheckup(1074-1074)correlatedUrl(1071-1071)client(1114-1114)
🔇 Additional comments (3)
src/index.ts (3)
489-493: LGTM! Timeout parameter correctly propagated.The
fetchExecutablemethod now accepts an optional timeout and passes it through tofetchArtifact, enabling per-call timeout customization.
575-587: LGTM! Client creation with timeout support.The
getClientmethod correctly accepts and propagates the timeout parameter toidsHost.getGot, allowing customization of request timeouts.
821-870: LGTM! Download file with timeout support.The
downloadFilemethod correctly accepts the timeout parameter and uses it when creating the Got client for streaming the download.
| timeout?: number, | ||
| ): Promise<Got> { | ||
| if (this.client === undefined) { | ||
| this.client = got.extend({ | ||
| timeout: { | ||
| request: DEFAULT_TIMEOUT, | ||
| request: timeout ?? DEFAULT_TIMEOUT, |
There was a problem hiding this comment.
Critical: Client caching breaks per-call timeout customization.
The client is cached in this.client (line 51) and reused for all subsequent calls. Once created with a specific timeout, that timeout persists for all future requests, ignoring any different timeout values passed to later getGot calls.
For example:
- First call:
getGot(undefined, 5000)creates client with 5s timeout - Second call:
getGot(undefined, 30000)reuses the same client (still 5s timeout)
This defeats the purpose of the timeout parameter.
Solution: Remove the client caching or make the cache timeout-aware:
Option 1 (Recommended): Remove caching
async getGot(
recordFailoverCallback?: (
incitingError: unknown,
prevUrl: URL,
nextUrl: URL,
) => void,
timeout?: number,
): Promise<Got> {
- if (this.client === undefined) {
- this.client = got.extend({
+ return got.extend({
timeout: {
request: timeout ?? DEFAULT_TIMEOUT,
},
retry: {
limit: Math.max((await this.getUrlsByPreference()).length, 3),
methods: ["GET", "HEAD"],
},
hooks: {
beforeRetry: [
async (error, retryCount) => {
const prevUrl = await this.getRootUrl();
this.markCurrentHostBroken();
const nextUrl = await this.getRootUrl();
if (recordFailoverCallback !== undefined) {
recordFailoverCallback(error, prevUrl, nextUrl);
}
actionsCore.info(
`Retrying after error ${error.code}, retry #: ${retryCount}`,
);
},
],
beforeRequest: [
async (options) => {
// The getter always returns a URL, even though the setter accepts a string
const currentUrl: URL = options.url as URL;
if (this.isUrlSubjectToDynamicUrls(currentUrl)) {
const newUrl: URL = new URL(currentUrl);
const url: URL = await this.getRootUrl();
newUrl.host = url.host;
options.url = newUrl;
actionsCore.debug(`Transmuted ${currentUrl} into ${newUrl}`);
} else {
actionsCore.debug(`No transmutations on ${currentUrl}`);
}
},
],
},
});
- }
-
- return this.client;
}Remove the client field from the constructor as well:
constructor(
idsProjectName: string,
diagnosticsSuffix: string | undefined,
runtimeDiagnosticsUrl: string | undefined,
) {
this.idsProjectName = idsProjectName;
this.diagnosticsSuffix = diagnosticsSuffix;
this.runtimeDiagnosticsUrl = runtimeDiagnosticsUrl;
- this.client = undefined;
}And remove the field declaration:
export class IdsHost {
private idsProjectName: string;
private diagnosticsSuffix?: string;
private runtimeDiagnosticsUrl?: string;
private prioritizedURLs?: URL[];
- private client?: Got;Option 2: Make cache timeout-aware (more complex)
Use a Map to cache clients by timeout value, but this adds complexity and may not be worth it if clients are lightweight to create.
🤖 Prompt for AI Agents
In src/ids-host.ts around lines 49 to 54, the method caches a single got client
on this.client and reuses it for all calls so a client created with one timeout
will incorrectly apply that timeout to subsequent calls; remove the persistent
client cache and instantiate a new got client per getGot call (or, if you prefer
caching, implement a timeout-aware cache keyed by timeout values) so that the
passed timeout parameter is respected for each request; also remove the client
field from the class (or change its type to a Map keyed by timeout) and update
any constructor references accordingly.
1 participant
Description
This change allows nix-installer-action etc. to avoid hard-coding a 10-second timeout for fetching Nix binaries.
Checklist
Summary by CodeRabbit