Skip to content

Commit

Permalink
fix(node-http-handler): call socket.setTimeout instead of request.set…
Browse files Browse the repository at this point in the history
…Timeout if socket established (smithy-lang#1477)
  • Loading branch information
kuhe authored Dec 23, 2024
1 parent 35aad30 commit 5e73108
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/short-onions-shop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@smithy/node-http-handler": patch
---

fix delayed calling of setTimeout on requests
25 changes: 25 additions & 0 deletions packages/node-http-handler/src/set-socket-timeout.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,31 @@ describe("setSocketTimeout", () => {
expect(clientRequest.setTimeout).toHaveBeenLastCalledWith(0, expect.any(Function));
});

describe("event listener registration deferral", () => {
const clientRequestWithSocket: any = {
destroy: vi.fn(),
setTimeout: vi.fn(),
socket: {
setTimeout: vi.fn(),
},
};

it("calls setTimeout on the socket if it is available after deferral", async () => {
const eventListenerMinimumTimeoutToDefer = 6000;
const deferralTimeout = 3000;
const expectedDeferredSocketTimeout = eventListenerMinimumTimeoutToDefer - deferralTimeout;
setSocketTimeout(clientRequestWithSocket, vi.fn(), eventListenerMinimumTimeoutToDefer);

vi.runAllTimers();

expect(clientRequestWithSocket.socket.setTimeout).toHaveBeenCalledTimes(1);
expect(clientRequestWithSocket.socket.setTimeout).toHaveBeenLastCalledWith(
expectedDeferredSocketTimeout,
expect.any(Function)
);
});
});

it(`destroys the request on timeout`, () => {
setSocketTimeout(clientRequest, vi.fn(), 1);
expect(clientRequest.destroy).not.toHaveBeenCalled();
Expand Down
12 changes: 9 additions & 3 deletions packages/node-http-handler/src/set-socket-timeout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,17 @@ export const setSocketTimeout = (
timeoutInMs = 0
): NodeJS.Timeout | number => {
const registerTimeout = (offset: number) => {
request.setTimeout(timeoutInMs - offset, () => {
// destroy the request
const timeout = timeoutInMs - offset;
const onTimeout = () => {
request.destroy();
reject(Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" }));
});
};

if (request.socket) {
request.socket.setTimeout(timeout, onTimeout);
} else {
request.setTimeout(timeout, onTimeout);
}
};

if (0 < timeoutInMs && timeoutInMs < 6000) {
Expand Down

0 comments on commit 5e73108

Please sign in to comment.