fix(executor-http): prevent potential leak

ardatan · Jul 5, 2024 · 46eab79 · 46eab79
1 parent b0ffac8
commit 46eab79
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 59 deletions.
diff --git a/.changeset/great-flies-shake.md b/.changeset/great-flies-shake.md
@@ -0,0 +1,5 @@
+---
+"@graphql-tools/executor-http": patch
+---
+
+Fixed potential leak on executor disposal
diff --git a/packages/executors/http/src/createTimeoutSignal.ts b/packages/executors/http/src/createTimeoutSignal.ts
diff --git a/packages/executors/http/src/index.ts b/packages/executors/http/src/index.ts
@@ -109,13 +109,12 @@ export function buildHTTPExecutor(
   options?: HTTPExecutorOptions,
 ): Executor<any, HTTPExecutorOptions> {
   const printFn = options?.print ?? defaultPrintFn;
-  const controller = new AbortController();
+  const disposeCtrl = new AbortController();
   const executor = (request: ExecutionRequest<any, any, any, HTTPExecutorOptions>) => {
-    if (controller.signal.aborted) {
+    if (disposeCtrl.signal.aborted) {
       throw new Error('Executor was disposed. Aborting execution');
     }
     const fetchFn = request.extensions?.fetch ?? options?.fetch ?? defaultFetch;
-    let signal = controller.signal;
     let method = request.extensions?.method || options?.method;
 
     const operationAst = getOperationASTFromRequest(request);
@@ -154,10 +153,21 @@ export function buildHTTPExecutor(
 
     const query = printFn(request.document);
 
+    let signal = disposeCtrl.signal;
+    let clearTimeoutFn: VoidFunction = () => {};
     if (options?.timeout) {
-      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-      // @ts-ignore AbortSignal.any is not yet in the DOM types
-      signal = AbortSignal.any([signal, AbortSignal.timeout(options.timeout)]);
+      const timeoutCtrl = new AbortController();
+      signal = timeoutCtrl.signal;
+      disposeCtrl.signal.addEventListener('abort', clearTimeoutFn);
+      const timeoutId = setTimeout(() => {
+        if (!timeoutCtrl.signal.aborted) {
+          timeoutCtrl.abort('timeout');
+        }
+        disposeCtrl.signal.removeEventListener('abort', clearTimeoutFn);
+      }, options.timeout);
+      clearTimeoutFn = () => {
+        clearTimeout(timeoutId);
+      };
     }
 
     const responseDetailsForError: {
@@ -222,6 +232,8 @@ export function buildHTTPExecutor(
         responseDetailsForError.status = fetchResult.status;
         responseDetailsForError.statusText = fetchResult.statusText;
 
+        clearTimeoutFn();
+
         // Retry should respect HTTP Errors
         if (options?.retry != null && !fetchResult.status.toString().startsWith('2')) {
           throw new Error(fetchResult.statusText || `HTTP Error: ${fetchResult.status}`);
@@ -417,11 +429,11 @@ export function buildHTTPExecutor(
   const disposableExecutor: DisposableExecutor = executor;
 
   disposableExecutor[Symbol.dispose] = () => {
-    return controller.abort(new Error('Executor was disposed. Aborting execution'));
+    return disposeCtrl.abort(new Error('Executor was disposed. Aborting execution'));
   };
 
   disposableExecutor[Symbol.asyncDispose] = () => {
-    return controller.abort(new Error('Executor was disposed. Aborting execution'));
+    return disposeCtrl.abort(new Error('Executor was disposed. Aborting execution'));
   };
 
   return disposableExecutor;

diff --git a/packages/executors/http/tests/retry-timeout.test.ts b/packages/executors/http/tests/retry-timeout.test.ts
@@ -93,14 +93,7 @@ describe('Retry & Timeout', () => {
     });
   });
   it('timeout', async () => {
-    server = new Server((req, res) => {
-      const timeout = setTimeout(() => {
-        res.end(JSON.stringify({ data: { hello: 'world' } }));
-      }, 1000);
-      req.once('close', () => {
-        clearTimeout(timeout);
-      });
-    });
+    server = new Server();
     server.listen(0);
     const executor = buildHTTPExecutor({
       endpoint: `http://localhost:${(server.address() as AddressInfo).port}`,
@@ -116,53 +109,45 @@ describe('Retry & Timeout', () => {
     expect(result).toMatchObject({
       errors: [
         {
-          message:
-            'The operation was aborted. reason: TimeoutError: The operation was aborted due to timeout',
+          message: 'The operation was aborted. reason: timeout',
         },
       ],
     });
   });
-  if (!process.env['LEAK_TEST']) {
-    it('retry & timeout', async () => {
-      let cnt = 0;
-      server = new Server((req, res) => {
-        if (cnt < 2) {
-          cnt++;
-          const timeout = setTimeout(() => {
-            res.end(JSON.stringify({ errors: [{ message: `error in ${cnt}` }] }));
-          }, 1000);
-          req.once('close', () => {
-            clearTimeout(timeout);
-          });
-        } else {
-          res.end(JSON.stringify({ data: { hello: 'world' } }));
-        }
-      });
-      server.on('connection', socket => {
-        sockets.add(socket);
-        socket.once('close', () => {
-          sockets.delete(socket);
-        });
-      });
-      server.listen(0);
-      const executor = buildHTTPExecutor({
-        endpoint: `http://localhost:${(server.address() as AddressInfo).port}`,
-        timeout: 500,
-        retry: 3,
-      });
-      const result = await executor({
-        document: parse(/* GraphQL */ `
-          query {
-            hello
-          }
-        `),
-      });
-      expect(cnt).toEqual(2);
-      expect(result).toMatchObject({
-        data: {
-          hello: 'world',
-        },
+  it('retry & timeout', async () => {
+    let cnt = 0;
+    server = new Server((_, res) => {
+      if (cnt < 2) {
+        cnt++;
+      } else {
+        res.end(JSON.stringify({ data: { hello: 'world' } }));
+      }
+    });
+    server.on('connection', socket => {
+      sockets.add(socket);
+      socket.once('close', () => {
+        sockets.delete(socket);
       });
     });
-  }
+    server.listen(0);
+    const executor = buildHTTPExecutor({
+      endpoint: `http://localhost:${(server.address() as AddressInfo).port}`,
+      timeout: 500,
+      retry: 3,
+    });
+    const result = await executor({
+      document: parse(/* GraphQL */ `
+        query {
+          hello
+        }
+      `),
+    });
+    expect(result).toMatchObject({
+      data: {
+        hello: 'world',
+      },
+    });
+    expect(cnt).toEqual(2);
+    await executor[Symbol.asyncDispose]?.();
+  });
 });