Skip to content

Commit 9f74bc9

Browse files
authored
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.
1 parent 1f0958f commit 9f74bc9

File tree

12 files changed

+290
-5
lines changed

12 files changed

+290
-5
lines changed

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
1212

13-
Work in this release was contributed by @nwalters512, @aloisklink, @arturovt, @benjick and @maximepvrt. Thank you for your contributions!
13+
Work in this release was contributed by @nwalters512, @aloisklink, @arturovt, @benjick, @maximepvrt, and @mstrokin. Thank you for your contributions!
1414

1515
- **feat(solidstart)!: Default to `--import` setup and add `autoInjectServerSentry` ([#14862](https://github.com/getsentry/sentry-javascript/pull/14862))**
1616

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { join } from 'path';
2-
import { createRunner } from '../../utils/runner';
2+
import { createRunner } from '../../../utils/runner';
33

44
describe('ContextLines integration in ESM', () => {
55
test('reads encoded context lines from filenames with spaces', done => {
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import * as Sentry from '@sentry/node';
2+
3+
export function captureException(i: number): void {
4+
Sentry.captureException(new Error(`error in loop ${i}`));
5+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import { captureException } from './nested-file';
2+
3+
export function runSentry(): void {
4+
for (let i = 0; i < 10; i++) {
5+
captureException(i);
6+
}
7+
}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { execSync } from 'node:child_process';
2+
import * as path from 'node:path';
3+
4+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
5+
import * as Sentry from '@sentry/node';
6+
7+
Sentry.init({
8+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
9+
release: '1.0',
10+
transport: loggingTransport,
11+
});
12+
13+
import { runSentry } from './other-file';
14+
15+
runSentry();
16+
17+
const lsofOutput = execSync(`lsof -p ${process.pid}`, { encoding: 'utf8' });
18+
const lsofTable = lsofOutput.split('\n');
19+
const mainPath = __dirname.replace(`${path.sep}suites${path.sep}contextLines${path.sep}memory-leak`, '');
20+
const numberOfLsofEntriesWithMainPath = lsofTable.filter(entry => entry.includes(mainPath));
21+
22+
// There should only be a single entry with the main path, otherwise we are leaking file handles from the
23+
// context lines integration.
24+
if (numberOfLsofEntriesWithMainPath.length > 1) {
25+
// eslint-disable-next-line no-console
26+
console.error('Leaked file handles detected');
27+
// eslint-disable-next-line no-console
28+
console.error(lsofTable);
29+
process.exit(1);
30+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
2+
3+
describe('ContextLines integration in CJS', () => {
4+
afterAll(() => {
5+
cleanupChildProcesses();
6+
});
7+
8+
// Regression test for: https://github.com/getsentry/sentry-javascript/issues/14892
9+
test('does not leak open file handles', done => {
10+
createRunner(__dirname, 'scenario.ts')
11+
.expectN(10, {
12+
event: {},
13+
})
14+
.start(done);
15+
});
16+
});

dev-packages/node-integration-tests/test.txt

Lines changed: 213 additions & 0 deletions
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)