fix(node): Enforce that ContextLines integration does not leave open …

…file handles (#14995)

The ContextLines integration uses readable streams to more memory
efficiently read files that it uses to attach source context to outgoing
events. See more details here:
#12221

Unfortunately, if we don't explicitly destroy the stream after creating
and using it, it won't get closed, even when we remove the readline
interface that uses the stream (which actual does the reading of files).

To fix this, we adjust the resolve logic when getting file context to
destroy the stream, as we anyway are done with the readline interface.

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 @nwalters512, @aloisklink, @arturovt, @benjick and @maximepvrt. Thank you for your contributions!
+Work in this release was contributed by @nwalters512, @aloisklink, @arturovt, @benjick, @maximepvrt, and @mstrokin. Thank you for your contributions!
 
 - **feat(solidstart)!: Default to `--import` setup and add `autoInjectServerSentry` ([#14862](https://github.com/getsentry/sentry-javascript/pull/14862))**
 

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

@@ -1,5 +1,5 @@
 import { join } from 'path';
-import { createRunner } from '../../utils/runner';
+import { createRunner } from '../../../utils/runner';
 
 describe('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://[email protected]/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,16 @@
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+describe('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++;