feat(node-http-handler): update timeout code and tests (#1691)

Author: trivikr

packages/node-http-handler/src/set-connection-timeout.spec.ts

@@ -1,32 +1,94 @@
-import { ClientRequest } from "http";
-
 import { setConnectionTimeout } from "./set-connection-timeout";
 
 describe("setConnectionTimeout", () => {
+  const reject = jest.fn();
   const clientRequest: any = {
     on: jest.fn(),
+    destroy: jest.fn(),
   };
 
   beforeEach(() => {
-    clientRequest.on.mockClear();
+    jest.clearAllMocks();
   });
 
   it("will not attach listeners if timeout is 0", () => {
-    setConnectionTimeout(<ClientRequest>clientRequest, jest.fn(), 0);
-
-    expect(clientRequest.on.mock.calls.length).toBe(0);
+    setConnectionTimeout(clientRequest, reject, 0);
+    expect(clientRequest.on).not.toHaveBeenCalled();
   });
 
   it("will not attach listeners if timeout is not provided", () => {
-    setConnectionTimeout(<ClientRequest>clientRequest, jest.fn());
-
-    expect(clientRequest.on.mock.calls.length).toBe(0);
+    setConnectionTimeout(clientRequest, reject);
+    expect(clientRequest.on).not.toHaveBeenCalled();
   });
 
-  it("will attach listeners if timeout is provided", () => {
-    setConnectionTimeout(<ClientRequest>clientRequest, jest.fn(), 100);
+  describe("when timeout is provided", () => {
+    const timeoutInMs = 100;
+    const mockSocket = {
+      connecting: true,
+      on: jest.fn(),
+    };
+
+    beforeEach(() => {
+      jest.useFakeTimers();
+      setConnectionTimeout(clientRequest, reject, timeoutInMs);
+    });
+
+    it("attaches listener", () => {
+      expect(clientRequest.on).toHaveBeenCalledTimes(1);
+      expect(clientRequest.on).toHaveBeenCalledWith("socket", expect.any(Function));
+    });
+
+    it("doesn't set timeout if socket is already connected", () => {
+      clientRequest.on.mock.calls[0][1]({
+        ...mockSocket,
+        connecting: false,
+      });
+      expect(mockSocket.on).not.toHaveBeenCalled();
+      expect(setTimeout).not.toHaveBeenCalled();
+      expect(reject).not.toHaveBeenCalled();
+    });
+
+    it("rejects and aborts request if socket isn't connected by timeout", () => {
+      clientRequest.on.mock.calls[0][1](mockSocket);
+      expect(setTimeout).toHaveBeenCalledTimes(1);
+      expect(setTimeout).toHaveBeenCalledWith(expect.any(Function), timeoutInMs);
+      expect(mockSocket.on).toHaveBeenCalledTimes(1);
+      expect(mockSocket.on).toHaveBeenCalledWith("connect", expect.any(Function));
+
+      expect(clientRequest.destroy).not.toHaveBeenCalled();
+      expect(reject).not.toHaveBeenCalled();
+
+      // Fast-forward until timer has been executed.
+      jest.advanceTimersByTime(timeoutInMs);
+      expect(clientRequest.destroy).toHaveBeenCalledTimes(1);
+      expect(reject).toHaveBeenCalledTimes(1);
+      expect(reject).toHaveBeenCalledWith(
+        Object.assign(new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`), {
+          name: "TimeoutError",
+        })
+      );
+    });
+
+    it("clears timeout if socket gets connected", () => {
+      const mockTimeoutId = 42;
+      ((setTimeout as unknown) as jest.Mock).mockReturnValueOnce(mockTimeoutId);
+      clientRequest.on.mock.calls[0][1](mockSocket);
+
+      expect(clientRequest.destroy).not.toHaveBeenCalled();
+      expect(reject).not.toHaveBeenCalled();
+      expect(clearTimeout).not.toHaveBeenCalled();
+
+      // Fast-forward for half the amount of time and call connect callback to clear timer.
+      jest.advanceTimersByTime(timeoutInMs / 2);
+      mockSocket.on.mock.calls[0][1]();
+
+      expect(clearTimeout).toHaveBeenCalled();
+      expect(clearTimeout).toHaveBeenCalledWith(mockTimeoutId);
 
-    expect(clientRequest.on.mock.calls.length).toBe(1);
-    expect(clientRequest.on.mock.calls[0][0]).toBe("socket");
+      // Fast-forward until timer has been executed.
+      jest.runAllTimers();
+      expect(clientRequest.destroy).not.toHaveBeenCalled();
+      expect(reject).not.toHaveBeenCalled();
+    });
   });
 });

packages/node-http-handler/src/set-connection-timeout.ts

@@ -1,27 +1,28 @@
 import { ClientRequest } from "http";
 import { Socket } from "net";
 
-export function setConnectionTimeout(request: ClientRequest, reject: (err: Error) => void, timeoutInMs = 0) {
+export const setConnectionTimeout = (request: ClientRequest, reject: (err: Error) => void, timeoutInMs = 0) => {
   if (!timeoutInMs) {
     return;
   }
 
-  request.on("socket", function (this: ClientRequest, socket: Socket) {
+  request.on("socket", (socket: Socket) => {
     if (socket.connecting) {
-      // Throw a connecting timeout error unless a connection is made within x time
+      // Throw a connecting timeout error unless a connection is made within x time.
       const timeoutId = setTimeout(() => {
-        // abort the request to destroy it
-        this.abort();
-
-        const timeoutError = new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`);
-        timeoutError.name = "TimeoutError";
-        reject(timeoutError);
+        // destroy the request.
+        request.destroy();
+        reject(
+          Object.assign(new Error(`Socket timed out without establishing a connection within ${timeoutInMs} ms`), {
+            name: "TimeoutError",
+          })
+        );
       }, timeoutInMs);
 
-      // if the connection was established, cancel the timeout
+      // if the connection was established, cancel the timeout.
       socket.on("connect", () => {
         clearTimeout(timeoutId);
       });
     }
   });
-}
+};

packages/node-http-handler/src/set-socket-timeout.spec.ts

@@ -1,56 +1,50 @@
-import { ClientRequest } from "http";
-
 import { setSocketTimeout } from "./set-socket-timeout";
 
 describe("setSocketTimeout", () => {
   const clientRequest: any = {
-    abort: jest.fn(),
+    destroy: jest.fn(),
     setTimeout: jest.fn(),
   };
 
   beforeEach(() => {
-    clientRequest.abort.mockClear();
-    clientRequest.setTimeout.mockClear();
+    jest.clearAllMocks();
   });
 
   it(`sets the request's timeout if provided`, () => {
-    setSocketTimeout(<ClientRequest>clientRequest, jest.fn(), 100);
+    setSocketTimeout(clientRequest, jest.fn(), 100);
 
-    expect(clientRequest.setTimeout.mock.calls.length).toBe(1);
-    expect(clientRequest.setTimeout.mock.calls[0][0]).toBe(100);
+    expect(clientRequest.setTimeout).toHaveBeenCalledTimes(1);
+    expect(clientRequest.setTimeout).toHaveBeenLastCalledWith(100, expect.any(Function));
   });
 
   it(`sets the request's timeout to 0 if not provided`, () => {
-    setSocketTimeout(<ClientRequest>clientRequest, jest.fn());
+    setSocketTimeout(clientRequest, jest.fn());
 
-    expect(clientRequest.setTimeout.mock.calls.length).toBe(1);
-    expect(clientRequest.setTimeout.mock.calls[0][0]).toBe(0);
+    expect(clientRequest.setTimeout).toHaveBeenCalledTimes(1);
+    expect(clientRequest.setTimeout).toHaveBeenLastCalledWith(0, expect.any(Function));
   });
 
-  it(`aborts the request on timeout`, () => {
-    setSocketTimeout(<ClientRequest>clientRequest, jest.fn());
-
-    expect(clientRequest.setTimeout.mock.calls.length).toBe(1);
-    expect(clientRequest.setTimeout.mock.calls[0][0]).toBe(0);
+  it(`destroys the request on timeout`, () => {
+    setSocketTimeout(clientRequest, jest.fn());
+    expect(clientRequest.destroy).not.toHaveBeenCalled();
 
     // call setTimeout callback
-    const cb = clientRequest.setTimeout.mock.calls[0][1];
-    cb.call(clientRequest);
-    expect(clientRequest.abort.mock.calls.length).toBe(1);
+    clientRequest.setTimeout.mock.calls[0][1]();
+    expect(clientRequest.destroy).toHaveBeenCalledTimes(1);
   });
 
   it(`rejects on timeout with a TimeoutError`, () => {
     const reject = jest.fn();
+    const timeoutInMs = 100;
 
-    setSocketTimeout(<ClientRequest>clientRequest, reject);
-
-    expect(clientRequest.setTimeout.mock.calls.length).toBe(1);
-    expect(clientRequest.setTimeout.mock.calls[0][0]).toBe(0);
+    setSocketTimeout(clientRequest, reject, timeoutInMs);
+    expect(reject).not.toHaveBeenCalled();
 
     // call setTimeout callback
-    const cb = clientRequest.setTimeout.mock.calls[0][1];
-    cb.call(clientRequest);
-    expect(reject.mock.calls.length).toBe(1);
-    expect(reject.mock.calls[0][0].name).toBe("TimeoutError");
+    clientRequest.setTimeout.mock.calls[0][1]();
+    expect(reject).toHaveBeenCalledTimes(1);
+    expect(reject).toHaveBeenCalledWith(
+      Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" })
+    );
   });
 });

packages/node-http-handler/src/set-socket-timeout.ts

@@ -1,11 +1,9 @@
 import { ClientRequest } from "http";
 
-export function setSocketTimeout(request: ClientRequest, reject: (err: Error) => void, timeoutInMs = 0) {
-  request.setTimeout(timeoutInMs, function (this: ClientRequest) {
-    // abort the request to destroy it
-    this.abort();
-    const timeoutError = new Error(`Connection timed out after ${timeoutInMs} ms`);
-    timeoutError.name = "TimeoutError";
-    reject(timeoutError);
+export const setSocketTimeout = (request: ClientRequest, reject: (err: Error) => void, timeoutInMs = 0) => {
+  request.setTimeout(timeoutInMs, () => {
+    // destroy the request
+    request.destroy();
+    reject(Object.assign(new Error(`Connection timed out after ${timeoutInMs} ms`), { name: "TimeoutError" }));
   });
-}
+};