Skip to content

Commit c4e923a

Browse files
authored
feat(http-handlers): add support for per-request/per-operation timeouts (#1636)
* feat(handlers): add support for per-request timeouts * chore: changeset * chore: formatting * fix(node-http-handler): explicitly type options obj
1 parent 7c3ee84 commit c4e923a

File tree

7 files changed

+147
-13
lines changed

7 files changed

+147
-13
lines changed

.changeset/gentle-balloons-drum.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@smithy/fetch-http-handler": minor
3+
"@smithy/node-http-handler": minor
4+
---
5+
6+
per-request timeouts support

packages/fetch-http-handler/src/fetch-http-handler.spec.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,37 @@ const globalFetch = global.fetch;
301301
expect(mockFetch.mock.calls.length).toBe(1);
302302
});
303303

304+
describe("per-request requestTimeout", () => {
305+
it("should use per-request timeout over handler config timeout", async () => {
306+
const mockFetch = vi.fn(() => new Promise(() => {})); // never resolve
307+
(global as any).fetch = mockFetch;
308+
309+
const fetchHttpHandler = new FetchHttpHandler({ requestTimeout: 5000 });
310+
311+
const start = Date.now();
312+
await expect(fetchHttpHandler.handle({} as any, { requestTimeout: 50 })).rejects.toHaveProperty(
313+
"name",
314+
"TimeoutError"
315+
);
316+
317+
const elapsed = Date.now() - start;
318+
expect(elapsed).toBeLessThan(100); // should timeout quickly
319+
});
320+
321+
it("should fall back to handler config timeout when per-request timeout not provided", async () => {
322+
const mockFetch = vi.fn(() => new Promise(() => {}));
323+
(global as any).fetch = mockFetch;
324+
325+
const fetchHttpHandler = new FetchHttpHandler({ requestTimeout: 50 });
326+
327+
const start = Date.now();
328+
await expect(fetchHttpHandler.handle({} as any, {})).rejects.toHaveProperty("name", "TimeoutError");
329+
330+
const elapsed = Date.now() - start;
331+
expect(elapsed).toBeLessThan(100);
332+
});
333+
});
334+
304335
it("will throw timeout error it timeout finishes before request", async () => {
305336
const mockFetch = vi.fn(() => {
306337
return new Promise(() => {});

packages/fetch-http-handler/src/fetch-http-handler.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { FetchHttpHandlerOptions } from "@smithy/types";
44
import { HeaderBag, HttpHandlerOptions, Provider } from "@smithy/types";
55

66
import { createRequest } from "./create-request";
7-
import { requestTimeout } from "./request-timeout";
7+
import { requestTimeout as requestTimeoutFn } from "./request-timeout";
88

99
declare let AbortController: any;
1010

@@ -73,11 +73,14 @@ export class FetchHttpHandler implements HttpHandler<FetchHttpHandlerOptions> {
7373
// Do nothing. TLS and HTTP/2 connection pooling is handled by the browser.
7474
}
7575

76-
async handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> {
76+
async handle(
77+
request: HttpRequest,
78+
{ abortSignal, requestTimeout }: HttpHandlerOptions = {}
79+
): Promise<{ response: HttpResponse }> {
7780
if (!this.config) {
7881
this.config = await this.configProvider;
7982
}
80-
const requestTimeoutInMs = this.config!.requestTimeout;
83+
const requestTimeoutInMs = requestTimeout ?? this.config!.requestTimeout;
8184
const keepAlive = this.config!.keepAlive === true;
8285
const credentials = this.config!.credentials as RequestInit["credentials"];
8386

@@ -175,7 +178,7 @@ export class FetchHttpHandler implements HttpHandler<FetchHttpHandlerOptions> {
175178
}),
176179
};
177180
}),
178-
requestTimeout(requestTimeoutInMs),
181+
requestTimeoutFn(requestTimeoutInMs),
179182
];
180183
if (abortSignal) {
181184
raceOfPromises.push(

packages/node-http-handler/src/node-http-handler.spec.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,4 +258,51 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
258258
);
259259
});
260260
});
261+
262+
describe("per-request requestTimeout", () => {
263+
it("should use per-request timeout over handler config timeout", () => {
264+
const nodeHttpHandler = new NodeHttpHandler({ requestTimeout: 5000 });
265+
const mockHandle = vi.spyOn(nodeHttpHandler, "handle");
266+
const testTimeout = (handlerTimeout: number, requestTimeout?: number) => {
267+
const handler = new NodeHttpHandler({ requestTimeout: handlerTimeout });
268+
const options = requestTimeout !== undefined ? { requestTimeout } : {};
269+
const expectedTimeout = requestTimeout ?? handlerTimeout;
270+
return expectedTimeout;
271+
};
272+
273+
// per-request timeout takes precedence
274+
expect(testTimeout(5000, 100)).toBe(100);
275+
276+
// fallback to handler config
277+
expect(testTimeout(200, undefined)).toBe(200);
278+
expect(testTimeout(200)).toBe(200);
279+
});
280+
281+
it("should pass correct timeout values to internal functions", async () => {
282+
const nodeHttpHandler = new NodeHttpHandler({ requestTimeout: 5000 });
283+
(nodeHttpHandler as any).config = {
284+
requestTimeout: 5000,
285+
httpAgent: new http.Agent(),
286+
httpsAgent: new https.Agent(),
287+
logger: console,
288+
};
289+
290+
const httpRequest = new HttpRequest({
291+
hostname: "example.com",
292+
method: "GET",
293+
protocol: "http:",
294+
path: "/",
295+
headers: {},
296+
});
297+
298+
const options1 = { requestTimeout: 100 };
299+
const options2: { requestTimeout?: number } = {};
300+
301+
const effectiveTimeout1 = options1.requestTimeout ?? (nodeHttpHandler as any).config.requestTimeout;
302+
const effectiveTimeout2 = options2.requestTimeout ?? (nodeHttpHandler as any).config.requestTimeout;
303+
304+
expect(effectiveTimeout1).toBe(100); // per-request timeout
305+
expect(effectiveTimeout2).toBe(5000); // handler config timeout
306+
});
307+
});
261308
});

packages/node-http-handler/src/node-http-handler.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,10 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
152152
this.config?.httpsAgent?.destroy();
153153
}
154154

155-
async handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> {
155+
async handle(
156+
request: HttpRequest,
157+
{ abortSignal, requestTimeout }: HttpHandlerOptions = {}
158+
): Promise<{ response: HttpResponse }> {
156159
if (!this.config) {
157160
this.config = await this.configProvider;
158161
}
@@ -281,8 +284,9 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
281284

282285
// Defer registration of socket event listeners if the connection and request timeouts
283286
// are longer than a few seconds. This avoids slowing down faster operations.
287+
const effectiveRequestTimeout = requestTimeout ?? this.config.requestTimeout;
284288
timeouts.push(setConnectionTimeout(req, reject, this.config.connectionTimeout));
285-
timeouts.push(setSocketTimeout(req, reject, this.config.requestTimeout));
289+
timeouts.push(setSocketTimeout(req, reject, effectiveRequestTimeout));
286290

287291
// Workaround for bug report in Node.js https://github.com/nodejs/node/issues/47137
288292
const httpAgent = nodeHttpsOptions.agent;
@@ -297,7 +301,7 @@ or increase socketAcquisitionWarningTimeout=(millis) in the NodeHttpHandler conf
297301
);
298302
}
299303

300-
writeRequestBodyPromise = writeRequestBody(req, request, this.config.requestTimeout).catch((e) => {
304+
writeRequestBodyPromise = writeRequestBody(req, request, effectiveRequestTimeout).catch((e) => {
301305
timeouts.forEach(timing.clearTimeout);
302306
return _reject(e);
303307
});

packages/node-http-handler/src/node-http2-handler.spec.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,45 @@ describe(NodeHttp2Handler.name, () => {
565565
);
566566
});
567567

568+
describe("per-request requestTimeout", () => {
569+
it("should use per-request timeout over handler config timeout", async () => {
570+
const nodeH2Handler = new NodeHttp2Handler({ requestTimeout: 5000 });
571+
572+
const mockH2Server = mockH2Servers[port1];
573+
mockH2Server.removeAllListeners("request");
574+
mockH2Server.on("request", () => {
575+
// don't respond - let it timeout
576+
});
577+
578+
const mockRequest = new HttpRequest(getMockReqOptions());
579+
580+
const start = Date.now();
581+
await expect(nodeH2Handler.handle(mockRequest, { requestTimeout: 100 })).rejects.toHaveProperty(
582+
"name",
583+
"TimeoutError"
584+
);
585+
586+
const elapsed = Date.now() - start;
587+
expect(elapsed).toBeLessThan(200);
588+
});
589+
590+
it("should fall back to handler config timeout when per-request timeout not provided", async () => {
591+
const nodeH2Handler = new NodeHttp2Handler({ requestTimeout: 100 });
592+
593+
const mockH2Server = mockH2Servers[port1];
594+
mockH2Server.removeAllListeners("request");
595+
mockH2Server.on("request", () => {});
596+
597+
const mockRequest = new HttpRequest(getMockReqOptions());
598+
599+
const start = Date.now();
600+
await expect(nodeH2Handler.handle(mockRequest, {})).rejects.toHaveProperty("name", "TimeoutError");
601+
602+
const elapsed = Date.now() - start;
603+
expect(elapsed).toBeLessThan(200);
604+
});
605+
});
606+
568607
describe.each([
569608
["object provider", async () => ({ disableConcurrentStreams: true })],
570609
["static object", { disableConcurrentStreams: true }],

packages/node-http-handler/src/node-http2-handler.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,19 @@ export class NodeHttp2Handler implements HttpHandler<NodeHttp2HandlerOptions> {
8787
this.connectionManager.destroy();
8888
}
8989

90-
async handle(request: HttpRequest, { abortSignal }: HttpHandlerOptions = {}): Promise<{ response: HttpResponse }> {
90+
async handle(
91+
request: HttpRequest,
92+
{ abortSignal, requestTimeout }: HttpHandlerOptions = {}
93+
): Promise<{ response: HttpResponse }> {
9194
if (!this.config) {
9295
this.config = await this.configProvider;
9396
this.connectionManager.setDisableConcurrentStreams(this.config.disableConcurrentStreams || false);
9497
if (this.config.maxConcurrentStreams) {
9598
this.connectionManager.setMaxConcurrentStreams(this.config.maxConcurrentStreams);
9699
}
97100
}
98-
const { requestTimeout, disableConcurrentStreams } = this.config;
101+
const { requestTimeout: configRequestTimeout, disableConcurrentStreams } = this.config;
102+
const effectiveRequestTimeout = requestTimeout ?? configRequestTimeout;
99103
return new Promise((_resolve, _reject) => {
100104
// It's redundant to track fulfilled because promises use the first resolution/rejection
101105
// but avoids generating unnecessary stack traces in the "close" event handler.
@@ -176,10 +180,10 @@ export class NodeHttp2Handler implements HttpHandler<NodeHttp2HandlerOptions> {
176180
}
177181
});
178182

179-
if (requestTimeout) {
180-
req.setTimeout(requestTimeout, () => {
183+
if (effectiveRequestTimeout) {
184+
req.setTimeout(effectiveRequestTimeout, () => {
181185
req.close();
182-
const timeoutError = new Error(`Stream timed out because of no activity for ${requestTimeout} ms`);
186+
const timeoutError = new Error(`Stream timed out because of no activity for ${effectiveRequestTimeout} ms`);
183187
timeoutError.name = "TimeoutError";
184188
rejectWithDestroy(timeoutError);
185189
});
@@ -227,7 +231,7 @@ export class NodeHttp2Handler implements HttpHandler<NodeHttp2HandlerOptions> {
227231
}
228232
});
229233

230-
writeRequestBodyPromise = writeRequestBody(req, request, requestTimeout);
234+
writeRequestBodyPromise = writeRequestBody(req, request, effectiveRequestTimeout);
231235
});
232236
}
233237

0 commit comments

Comments
 (0)