Skip to content

Commit 09ecd2e

Browse files
Justin-Nietzeljasnell
authored andcommitted
fs: added test for missing call to uv_fs_req_cleanup
Added a unit test for testing the memory usage of readFileSync. Test is looking specifically for the the issue caused by failing to free the filepath buffer in fs::ReadFileUtf8(), but it will also catch other significant memory leaks in readFileSync() as well. Refs: nodejs#57800 PR-URL: nodejs#57811 Fixes: nodejs#57800 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 6a7b005 commit 09ecd2e

File tree

1 file changed

+54
-0
lines changed

1 file changed

+54
-0
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (common.isIBMi)
4+
common.skip('On IBMi, the rss memory always returns zero');
5+
6+
// This test verifies that readFileSync does not leak memory when reading
7+
// UTF8 files. See: https://github.com/nodejs/node/issues/57800 for details.
8+
9+
const assert = require('node:assert');
10+
const fs = require('node:fs');
11+
const util = require('node:util');
12+
const tmpdir = require('../common/tmpdir');
13+
14+
// The memory leak being tested here was from a buffer with the absolute URI to
15+
// a file. For each file read, 2-4 bytes were (usually) leaked per character in
16+
// the URI. The length of the file path can be used to estimate the approximate
17+
// amount of memory that will be leaked if this issue is reintroduced. A longer
18+
// total path length will make the issue easier to test for. Some operating
19+
// systems like Windows have shorter default path length limits. If the path
20+
// is approaching that limit, the length of the path should be long enough to
21+
// effectively test for this memory leak.
22+
tmpdir.refresh();
23+
let testFile = tmpdir.resolve(
24+
'a-file-with-a-longer-than-usual-file-name-for-testing-a-memory-leak-related-to-path-length.txt',
25+
);
26+
if (testFile.length > process.env.MAX_PATH) {
27+
testFile = tmpdir.resolve('reasonable-length.txt');
28+
}
29+
30+
// The buffer being checked is WCHAR buffer. The size is going to be at least 2
31+
// bytes per character but can be more. Windows: 2; Mac: 2; Linux: 4 (usually);
32+
const iterations = 100_000;
33+
const minExpectedMemoryLeak = (testFile.length * 2) * iterations;
34+
35+
// This memory leak was exclusive to UTF8 encoded files.
36+
// Create our test file. The contents of the file don't matter.
37+
const options = { encoding: 'utf8' };
38+
fs.writeFileSync(testFile, '', options);
39+
40+
// Doing an initial big batch of file reads gives a more stable baseline memory
41+
// usage. Doing the same total iterations as the actual test isn't necessary.
42+
for (let i = 0; i < 100; i++) {
43+
fs.readFileSync(testFile, options);
44+
}
45+
const startMemory = process.memoryUsage();
46+
for (let i = 0; i < iterations; i++) {
47+
fs.readFileSync(testFile, options);
48+
}
49+
const endMemory = process.memoryUsage();
50+
51+
// Use 90% of the expected memory leak as the threshold just to be safe.
52+
const memoryDifference = endMemory.rss - startMemory.rss;
53+
assert.ok(memoryDifference < (minExpectedMemoryLeak * 0.9),
54+
`Unexpected memory overhead: ${util.inspect([startMemory, endMemory])}`);

0 commit comments

Comments
 (0)