test: use unique tmpdirs for each test

[joaocgreis]

Some of the recent failures in CI have happened because tests leave processes running or somehow block the tmpdir, causing the next test that tries to use the same tmpdir to fail. We use one tmpdir per thread of the test runner.

This changes the test system to use a different directory for each test, removing interference between tests.

With this, a test run will leave about 200 tmpdirs in test/, but these will be reused in future runs so they don't accumulate. Removing the tmpdir on process exit interferes with cleanup that some tests already do so it's not straightforward. For now, this doesn't create any false positives, only makes unrelated tests pass.

cc @nodejs/testing

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[Trott]

Since our tests don't typically clean up the tmp dirs, this is going to leave a lot of tmp dirs lying around. Maybe it makes sense to add a tmpdir cleanup process in test.py?

[Trott]

Some of the recent failures in CI have happened because tests leave processes running or somehow block the tmpdir, causing the next test that tries to use the same tmpdir to fail.

Is this not a bug that needs to be fixed? The first test should finish before the second test begins.

[mscdex]

I agree, fixing the problematic test is better than keeping hundreds of temporary directories around.

[joaocgreis]

Added code to clean the temp dir on process exit. Some tests already had cleanup on process exit that interferes with this, so I removed cleanup code from the tests. Also had to ensure every file descriptor is closed before the test completes.

The only test that actually requires a file to be left open at the end is addons/load-long-path, so I changed it to use a child process. The changes in other tests are fairly straightforward.

This makes the tests independent from each other concerning the temp dir. When a test leaves something running blocking the temp dir, it will no longer cause some other test to fail afterwards, but actually fail in the test that should fail. This will make it much easier to debug flaky tests.

Includes fixes for two Node issues that appeared while implementing this:

• Close the file descriptor for the promisified version of truncate: 119682e. The fix is similar to what is already there for lchmod, writeFile and readFile. @nodejs/fs
• Close the file descriptor for the REPL history file: 7f06449. @nodejs/repl

I can open these as separate PRs if needed. However, this PR would have to be blocked by those and the tests for the fixes is here.

It is tempting to clean the temp dirs on test.py instead of tmpdir.js, but I don't think that would be a good solution because: 1) test.py knows nothing about our temp dirs, it would be much better to keep this localized in tmpdir.js; 2) If there's any issue with the code in test.py, we break all the tests, not only the failing one; 3) I want to add some debug logging when removing the directory fails, ideally it should end up in the test report together with the error; 4) it wouldn't help us catch issues such as the ones above.

[Trott]

I realize we don't run these tests on Windows in CI, but there are tests in pummel (such as test-fs-largefile.js) that have an exit handler doing unlinkSync(). Should those be adjusted too?

[Trott]

I like this approach a lot. Thanks for doing this!

[joaocgreis]

tests in pummel... Should those be adjusted too?

Yes! Thanks for bringing this up!

[Trott]

Looks like the main issue here is that the tmp directories on Raspberry Pi devices in CI are NFS mounted and that can mess with the cleanup logic.

One possible solution workaround is to detect that it's an NFS file system and ignore cleanup errors in that situation. Another possibility is to ignore cleanup issues if they are on files of the format /^\.nfs\w+$/ or (perhaps more robust and easier to maintain) .startsWith('.nfs').

[joaocgreis]

@nodejs/benchmarking added a commit (1344243) to switch 3 benchmarks to common/tmpdir.js, to be able to run the benchmark test suite. There are other benchmarks that already use this.

@bnoordhuis, @indutny, @nodejs/streams: the only issue left here is that net.Server does not seem to close the file descriptor when listening on a pipe. This happens in both parallel/test-child-process-server-close and parallel/test-tls-wrap-econnreset-pipe. I confirmed by adding this to the tests:

const { execSync } = require('child_process');
const lsof = `lsof -p ${process.pid} | grep -e ^COMMAND -e tmp`;
process.on('exit', () => {
  const openFiles = execSync(lsof, { encoding: 'utf8' });
  console.error(`Open files (${lsof}):\n${openFiles}`);
});

Is this a real issue in Node? Since the server is closed in both tests shouldn't the file descriptor be closed? Or is this by design and we should run those tests in child processes like parallel/test-pipe-unref here?

@Trott I've only seen the .nfs* files when a file is actually open. This may be a real issue. For now, I've added a warning about this to the cleanup logic.

[Trott]

It would perhaps not be the most elegant solution, but maybe if .nfs* files are detected and nothing else, wait and check again?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24833/ ✔️

[joaocgreis]

Updated. Changed those two tests to use a child process. With this solution we don't lose anything, the tests keep testing exactly what they were testing before.

This is ready for review (hopefully final this time).

@Trott the .nfs* files won't go away as long as the file descriptors are open. So, they won't go away and the test would timeout. The files are also left open when not using NFS, but don't belong to any directory so don't block removing the directory. I made some code to show this happening on Linux: JaneaSystems@bbb42ef, but won't include it here because it makes the tests take 40 seconds longer to run (on my machine).

[Trott]

🎉

[Trott]

@refack Any idea if this might make the stuff you added a few months ago to the rimraf code in common unnecessary?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24913/

[Trott]

Landed in df936c5...8ef68e6

[cclauss]

Impressive work here! Congratulations.