fix(v8/node): Enforce that ContextLines integration does not leave op…

…en file handles (#14997)

v8 backport of #14995

CHANGELOG.md

@@ -10,7 +10,7 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
-Work in this release was contributed by @HHK1. Thank you for your contribution!
+Work in this release was contributed by @HHK1 and @mstrokin. Thank you for your contribution!
 
 ## 8.48.0
 

dev-packages/node-integration-tests/suites/contextLines/test.ts → dev-packages/node-integration-tests/suites/contextLines/filename-with-spaces/test.ts

@@ -1,6 +1,6 @@
 import { join } from 'path';
-import { conditionalTest } from '../../utils';
-import { createRunner } from '../../utils/runner';
+import { conditionalTest } from '../../../utils';
+import { createRunner } from '../../../utils/runner';
 
 conditionalTest({ min: 18 })('ContextLines integration in ESM', () => {
   test('reads encoded context lines from filenames with spaces', done => {

dev-packages/node-integration-tests/suites/contextLines/memory-leak/nested-file.ts

@@ -0,0 +1,5 @@
+import * as Sentry from '@sentry/node';
+
+export function captureException(i: number): void {
+  Sentry.captureException(new Error(`error in loop ${i}`));
+}

dev-packages/node-integration-tests/suites/contextLines/memory-leak/other-file.ts

@@ -0,0 +1,7 @@
+import { captureException } from './nested-file';
+
+export function runSentry(): void {
+  for (let i = 0; i < 10; i++) {
+    captureException(i);
+  }
+}

dev-packages/node-integration-tests/suites/contextLines/memory-leak/scenario.ts

@@ -0,0 +1,30 @@
+import { execSync } from 'node:child_process';
+import * as path from 'node:path';
+
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  transport: loggingTransport,
+});
+
+import { runSentry } from './other-file';
+
+runSentry();
+
+const lsofOutput = execSync(`lsof -p ${process.pid}`, { encoding: 'utf8' });
+const lsofTable = lsofOutput.split('\n');
+const mainPath = __dirname.replace(`${path.sep}suites${path.sep}contextLines${path.sep}memory-leak`, '');
+const numberOfLsofEntriesWithMainPath = lsofTable.filter(entry => entry.includes(mainPath));
+
+// There should only be a single entry with the main path, otherwise we are leaking file handles from the
+// context lines integration.
+if (numberOfLsofEntriesWithMainPath.length > 1) {
+  // eslint-disable-next-line no-console
+  console.error('Leaked file handles detected');
+  // eslint-disable-next-line no-console
+  console.error(lsofTable);
+  process.exit(1);
+}

dev-packages/node-integration-tests/suites/contextLines/memory-leak/test.ts

@@ -0,0 +1,17 @@
+import { conditionalTest } from '../../../utils';
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+conditionalTest({ min: 18 })('ContextLines integration in CJS', () => {
+  afterAll(() => {
+    cleanupChildProcesses();
+  });
+
+  // Regression test for: https://github.com/getsentry/sentry-javascript/issues/14892
+  test('does not leak open file handles', done => {
+    createRunner(__dirname, 'scenario.ts')
+      .expectN(10, {
+        event: {},
+      })
+      .start(done);
+  });
+});

dev-packages/node-integration-tests/utils/runner.ts

@@ -168,6 +168,12 @@ export function createRunner(...paths: string[]) {
       expectedEnvelopes.push(expected);
       return this;
     },
+    expectN: function (n: number, expected: Expected) {
+      for (let i = 0; i < n; i++) {
+        expectedEnvelopes.push(expected);
+      }
+      return this;
+    },
     expectHeader: function (expected: ExpectedEnvelopeHeader) {
       if (!expectedEnvelopeHeaders) {
         expectedEnvelopeHeaders = [];

packages/node/src/integrations/contextlines.ts

@@ -142,13 +142,21 @@ function getContextLinesFromFile(path: string, ranges: ReadlineRange[], output:
       input: stream,
     });
 
+    // We need to explicitly destroy the stream to prevent memory leaks,
+    // removing the listeners on the readline interface is not enough.
+    // See: https://github.com/nodejs/node/issues/9002 and https://github.com/getsentry/sentry-javascript/issues/14892
+    function destroyStreamAndResolve(): void {
+      stream.destroy();
+      resolve();
+    }
+
     // Init at zero and increment at the start of the loop because lines are 1 indexed.
     let lineNumber = 0;
     let currentRangeIndex = 0;
     const range = ranges[currentRangeIndex];
     if (range === undefined) {
       // We should never reach this point, but if we do, we should resolve the promise to prevent it from hanging.
-      resolve();
+      destroyStreamAndResolve();
       return;
     }
     let rangeStart = range[0];
@@ -162,14 +170,14 @@ function getContextLinesFromFile(path: string, ranges: ReadlineRange[], output:
       DEBUG_BUILD && logger.error(`Failed to read file: ${path}. Error: ${e}`);
       lineReaded.close();
       lineReaded.removeAllListeners();
-      resolve();
+      destroyStreamAndResolve();
     }
 
     // We need to handle the error event to prevent the process from crashing in < Node 16
     // https://github.com/nodejs/node/pull/31603
     stream.on('error', onStreamError);
     lineReaded.on('error', onStreamError);
-    lineReaded.on('close', resolve);
+    lineReaded.on('close', destroyStreamAndResolve);
 
     lineReaded.on('line', line => {
       lineNumber++;