From 7a65a6f700f00ec306960cc5a790647f26a8a7dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Thu, 25 Jul 2019 20:20:57 +0100 Subject: [PATCH 01/11] test: use unique tmpdirs for each test Tests can leave processes running blocking the tmpdir. This does not yet prevent tests from doing that, but prevents failures on subsequent tests. --- test/common/tmpdir.js | 3 ++- tools/test.py | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/test/common/tmpdir.js b/test/common/tmpdir.js index 33b2264a8d69f5..92bcc9521a8bdb 100644 --- a/test/common/tmpdir.js +++ b/test/common/tmpdir.js @@ -89,7 +89,8 @@ const testRoot = process.env.NODE_TEST_DIR ? // Using a `.` prefixed name, which is the convention for "hidden" on POSIX, // gets tools to ignore it by default or by simple rules, especially eslint. -const tmpdirName = '.tmp.' + (process.env.TEST_THREAD_ID || '0'); +const tmpdirName = '.tmp.' + + (process.env.TEST_SERIAL_ID || process.env.TEST_THREAD_ID || '0'); const tmpPath = path.join(testRoot, tmpdirName); function refresh(opts = {}) { diff --git a/tools/test.py b/tools/test.py index ff08749425eb5a..0726619bd092a0 100755 --- a/tools/test.py +++ b/tools/test.py @@ -77,6 +77,7 @@ class ProgressIndicator(object): def __init__(self, cases, flaky_tests_mode): self.cases = cases + self.serial_id = 0 self.flaky_tests_mode = flaky_tests_mode self.parallel_queue = Queue(len(cases)) self.sequential_queue = Queue(len(cases)) @@ -146,6 +147,8 @@ def RunSingle(self, parallel, thread_id): case = test case.thread_id = thread_id self.lock.acquire() + case.serial_id = self.serial_id + self.serial_id += 1 self.AboutToRun(case) self.lock.release() try: @@ -504,6 +507,7 @@ def __init__(self, context, path, arch, mode): self.mode = mode self.parallel = False self.disable_core_files = False + self.serial_id = 0 self.thread_id = 0 def IsNegative(self): @@ -535,6 +539,7 @@ def RunCommand(self, command, env): def Run(self): try: result = self.RunCommand(self.GetCommand(), { + "TEST_SERIAL_ID": "%d" % self.serial_id, "TEST_THREAD_ID": "%d" % self.thread_id, "TEST_PARALLEL" : "%d" % self.parallel }) From 326392f7f12ecd12ef6bc0f2ad099328e70e0954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 07:45:31 +0100 Subject: [PATCH 02/11] fs: close file descriptor of promisified truncate --- lib/internal/fs/promises.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 42dbbcc361ad99..d807cb71f7127f 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -264,7 +264,8 @@ async function rename(oldPath, newPath) { } async function truncate(path, len = 0) { - return ftruncate(await open(path, 'r+'), len); + const fd = await open(path, 'r+'); + return ftruncate(fd, len).finally(fd.close.bind(fd)); } async function ftruncate(handle, len = 0) { From 1d38c77c5898a5781c1e071b9c2650609b2d439c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 07:45:59 +0100 Subject: [PATCH 03/11] repl: close file descriptor of history file --- lib/internal/repl/history.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/internal/repl/history.js b/lib/internal/repl/history.js index dd3ea850954936..5d90b40e0041c7 100644 --- a/lib/internal/repl/history.js +++ b/lib/internal/repl/history.js @@ -93,6 +93,7 @@ function setupHistory(repl, historyPath, ready) { fs.ftruncate(hnd, 0, (err) => { repl._historyHandle = hnd; repl.on('line', online); + repl.once('exit', onexit); // Reading the file data out erases it repl.once('flushHistory', function() { @@ -137,6 +138,15 @@ function setupHistory(repl, historyPath, ready) { } } } + + function onexit() { + if (repl._flushing) { + repl.once('flushHistory', onexit); + return; + } + repl.off('line', online); + fs.close(repl._historyHandle, () => {}); + } } function _replHistoryMessage() { From a20031f511e6c3e7e36eba741a30878dbd8f03d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 16:55:15 +0100 Subject: [PATCH 04/11] benchmark: use test/common/tmpdir consistently Many benchmarks use test/common/tmpdir. This changes 3 benchmarks that use NODE_TMPDIR to also use test/common/tmpdir. This is necessary in preparation for the next commit that changes tmpdir to delete tmpdir.path when the Node.js process exits. Thus, if multiple benchmarks are run sequentially, the ones that use tmpdir will remove the directory and the ones changed here would fail because it does not exist. This happens when running test/benchmark. Note: to explicitly select a directory for tmpdir, use NODE_TEST_DIR. --- benchmark/fs/read-stream-throughput.js | 7 +++++-- benchmark/fs/readfile.js | 7 +++++-- benchmark/fs/write-stream-throughput.js | 7 +++++-- test/benchmark/test-benchmark-fs.js | 2 +- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/benchmark/fs/read-stream-throughput.js b/benchmark/fs/read-stream-throughput.js index cb5d98dc3279f6..4c8de582418960 100644 --- a/benchmark/fs/read-stream-throughput.js +++ b/benchmark/fs/read-stream-throughput.js @@ -3,11 +3,14 @@ const path = require('path'); const common = require('../common.js'); -const filename = path.resolve(process.env.NODE_TMPDIR || __dirname, - `.removeme-benchmark-garbage-${process.pid}`); const fs = require('fs'); const assert = require('assert'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); +const filename = path.resolve(tmpdir.path, + `.removeme-benchmark-garbage-${process.pid}`); + let encodingType, encoding, size, filesize; const bench = common.createBenchmark(main, { diff --git a/benchmark/fs/readfile.js b/benchmark/fs/readfile.js index 551804b1dbab1f..7460c8e233e59f 100644 --- a/benchmark/fs/readfile.js +++ b/benchmark/fs/readfile.js @@ -5,11 +5,14 @@ const path = require('path'); const common = require('../common.js'); -const filename = path.resolve(process.env.NODE_TMPDIR || __dirname, - `.removeme-benchmark-garbage-${process.pid}`); const fs = require('fs'); const assert = require('assert'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); +const filename = path.resolve(tmpdir.path, + `.removeme-benchmark-garbage-${process.pid}`); + const bench = common.createBenchmark(main, { dur: [5], len: [1024, 16 * 1024 * 1024], diff --git a/benchmark/fs/write-stream-throughput.js b/benchmark/fs/write-stream-throughput.js index 3936adc7ff1b42..44de0900d8ed15 100644 --- a/benchmark/fs/write-stream-throughput.js +++ b/benchmark/fs/write-stream-throughput.js @@ -3,10 +3,13 @@ const path = require('path'); const common = require('../common.js'); -const filename = path.resolve(process.env.NODE_TMPDIR || __dirname, - `.removeme-benchmark-garbage-${process.pid}`); const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); +const filename = path.resolve(tmpdir.path, + `.removeme-benchmark-garbage-${process.pid}`); + const bench = common.createBenchmark(main, { dur: [5], encodingType: ['buf', 'asc', 'utf'], diff --git a/test/benchmark/test-benchmark-fs.js b/test/benchmark/test-benchmark-fs.js index 7ae32fe617d58f..efb2c453bde77d 100644 --- a/test/benchmark/test-benchmark-fs.js +++ b/test/benchmark/test-benchmark-fs.js @@ -19,4 +19,4 @@ runBenchmark('fs', [ 'filesize=1024', 'dir=.github', 'withFileTypes=false' -], { NODE_TMPDIR: tmpdir.path, NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); +], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); From a686febf527c405f95295a537df0375e5bde36c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 07:50:21 +0100 Subject: [PATCH 05/11] test: clean tmpdir on process exit --- test/addons/load-long-path/test.js | 29 +++++++++++++++---- test/common/README.md | 6 ++++ test/common/tmpdir.js | 23 +++++++++++++++ test/parallel/test-fs-append-file-sync.js | 10 ------- test/parallel/test-fs-buffertype-writesync.js | 1 + test/parallel/test-fs-fsync.js | 1 + test/parallel/test-fs-long-path.js | 4 --- test/parallel/test-fs-options-immutable.js | 4 +-- ...est-fs-promises-file-handle-append-file.js | 4 +++ .../test-fs-promises-file-handle-chmod.js | 2 ++ .../test-fs-promises-file-handle-read.js | 4 +++ .../test-fs-promises-file-handle-readFile.js | 2 ++ .../test-fs-promises-file-handle-stat.js | 1 + .../test-fs-promises-file-handle-sync.js | 1 + .../test-fs-promises-file-handle-truncate.js | 2 ++ .../test-fs-promises-file-handle-write.js | 8 +++++ .../test-fs-promises-file-handle-writeFile.js | 2 ++ .../test-fs-promises-readfile-with-fd.js | 2 ++ .../test-fs-promises-writefile-with-fd.js | 2 ++ test/parallel/test-fs-promises.js | 7 +++++ test/parallel/test-fs-readdir-types.js | 1 + test/parallel/test-fs-readfile-fd.js | 4 +++ test/parallel/test-fs-readfile-pipe-large.js | 4 --- .../test-fs-readfilesync-pipe-large.js | 4 --- test/parallel/test-fs-ready-event-stream.js | 1 + test/parallel/test-fs-truncate-fd.js | 2 +- test/parallel/test-fs-truncate.js | 8 ++--- test/parallel/test-fs-write-file.js | 7 ----- .../test-fs-write-stream-change-open.js | 1 + test/parallel/test-fs-write-stream-err.js | 1 + .../test-fs-write-stream-throw-type-error.js | 8 ++--- test/parallel/test-fs-write-stream.js | 1 + test/parallel/test-fs-writefile-with-fd.js | 6 ++++ ...st-http2-respond-file-error-pipe-offset.js | 2 -- .../test-http2-respond-file-with-pipe.js | 2 -- .../test-internal-fs-syncwritestream.js | 2 ++ .../test-readline-async-iterators-destroy.js | 6 ++++ .../test-http2-timeout-large-write-file.js | 1 + 38 files changed, 126 insertions(+), 50 deletions(-) diff --git a/test/addons/load-long-path/test.js b/test/addons/load-long-path/test.js index 0160591746dc2e..168dad492b0255 100644 --- a/test/addons/load-long-path/test.js +++ b/test/addons/load-long-path/test.js @@ -6,9 +6,9 @@ if (common.isWindows && (process.env.PROCESSOR_ARCHITEW6432 !== undefined)) const fs = require('fs'); const path = require('path'); const assert = require('assert'); +const { fork } = require('child_process'); const tmpdir = require('../../common/tmpdir'); -tmpdir.refresh(); // Make a path that is more than 260 chars long. // Any given folder cannot have a name longer than 260 characters, @@ -17,7 +17,6 @@ let addonDestinationDir = path.resolve(tmpdir.path); for (let i = 0; i < 10; i++) { addonDestinationDir = path.join(addonDestinationDir, 'x'.repeat(30)); - fs.mkdirSync(addonDestinationDir); } const addonPath = path.join(__dirname, @@ -26,11 +25,29 @@ const addonPath = path.join(__dirname, 'binding.node'); const addonDestinationPath = path.join(addonDestinationDir, 'binding.node'); +// Loading an addon keeps the file open until the process terminates. Load +// the addon in a child process so that when the parent terminates the file +// is already closed and the tmpdir can be cleaned up. + +// Child +if (process.argv[2] === 'child') { + // Attempt to load at long path destination + const addon = require(addonDestinationPath); + assert.notStrictEqual(addon, null); + assert.strictEqual(addon.hello(), 'world'); + return; +} + +// Parent +tmpdir.refresh(); + // Copy binary to long path destination +fs.mkdirSync(addonDestinationDir, { recursive: true }); const contents = fs.readFileSync(addonPath); fs.writeFileSync(addonDestinationPath, contents); -// Attempt to load at long path destination -const addon = require(addonDestinationPath); -assert.notStrictEqual(addon, null); -assert.strictEqual(addon.hello(), 'world'); +// Run test +const child = fork(__filename, ['child'], { stdio: 'inherit' }); +child.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); +})); diff --git a/test/common/README.md b/test/common/README.md index 63c5fb1a56f0ec..3ad21ea4d0cc3d 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -906,6 +906,12 @@ The realpath of the testing temporary directory. Deletes and recreates the testing temporary directory. +The first time refresh is run it adds a listener to process `'exit'` that +cleans the temporary directory. Thus, every file under `tmpdir.path` needs to +be closed before the test completes. A good way to do this is to add a +listener to process `'beforeExit'`. If a file needs to be left open until Node +completes, use a child process and call `refresh` only in the parent. + ## WPT Module ### harness diff --git a/test/common/tmpdir.js b/test/common/tmpdir.js index 92bcc9521a8bdb..a71f5e95155ab8 100644 --- a/test/common/tmpdir.js +++ b/test/common/tmpdir.js @@ -61,6 +61,9 @@ function rimrafSync(pathname, { spawn = true } = {}) { } rmdirSync(pathname, e); } + + if (fs.existsSync(pathname)) + throw new Error(`Unable to rimraf ${pathname}`); } function rmdirSync(p, originalEr) { @@ -80,7 +83,9 @@ function rmdirSync(p, originalEr) { } }); fs.rmdirSync(p); + return; } + throw e; } } @@ -93,9 +98,27 @@ const tmpdirName = '.tmp.' + (process.env.TEST_SERIAL_ID || process.env.TEST_THREAD_ID || '0'); const tmpPath = path.join(testRoot, tmpdirName); +let firstRefresh = true; function refresh(opts = {}) { rimrafSync(this.path, opts); fs.mkdirSync(this.path); + + if (firstRefresh) { + firstRefresh = false; + // Clean only when a test uses refresh. This allows for child processes to + // use the tmpdir and only the parent will clean on exit. + process.on('exit', () => { + try { + // Change dit to avoid possible EBUSY + process.chdir(testRoot); + rimrafSync(tmpPath, { spawn: false }); + } catch (e) { + console.error('Can\'t clean tmpdir:', tmpPath); + console.error('Files blocking:', fs.readdirSync(tmpPath)); + throw e; + } + }); + } } module.exports = { diff --git a/test/parallel/test-fs-append-file-sync.js b/test/parallel/test-fs-append-file-sync.js index 190381234aa5ad..521776d77900ab 100644 --- a/test/parallel/test-fs-append-file-sync.js +++ b/test/parallel/test-fs-append-file-sync.js @@ -99,13 +99,3 @@ const fileData5 = fs.readFileSync(filename5); assert.strictEqual(Buffer.byteLength(data) + currentFileData.length, fileData5.length); - -// Exit logic for cleanup. - -process.on('exit', function() { - fs.unlinkSync(filename); - fs.unlinkSync(filename2); - fs.unlinkSync(filename3); - fs.unlinkSync(filename4); - fs.unlinkSync(filename5); -}); diff --git a/test/parallel/test-fs-buffertype-writesync.js b/test/parallel/test-fs-buffertype-writesync.js index d5257d214bdb81..042552a46f97b3 100644 --- a/test/parallel/test-fs-buffertype-writesync.js +++ b/test/parallel/test-fs-buffertype-writesync.js @@ -19,4 +19,5 @@ v.forEach((value) => { const fd = fs.openSync(filePath, 'w'); fs.writeSync(fd, value); assert.strictEqual(fs.readFileSync(filePath).toString(), String(value)); + fs.closeSync(fd); }); diff --git a/test/parallel/test-fs-fsync.js b/test/parallel/test-fs-fsync.js index cf80f46bd0238c..28a513d10f2d29 100644 --- a/test/parallel/test-fs-fsync.js +++ b/test/parallel/test-fs-fsync.js @@ -46,6 +46,7 @@ fs.open(fileTemp, 'a', 0o777, common.mustCall(function(err, fd) { assert.ifError(err); fs.fsync(fd, common.mustCall(function(err) { assert.ifError(err); + fs.closeSync(fd); })); })); })); diff --git a/test/parallel/test-fs-long-path.js b/test/parallel/test-fs-long-path.js index 11ec4d56fcea3f..3e61c9a769e117 100644 --- a/test/parallel/test-fs-long-path.js +++ b/test/parallel/test-fs-long-path.js @@ -49,7 +49,3 @@ fs.writeFile(fullPath, 'ok', common.mustCall(function(err) { assert.ifError(err); })); })); - -process.on('exit', function() { - fs.unlinkSync(fullPath); -}); diff --git a/test/parallel/test-fs-options-immutable.js b/test/parallel/test-fs-options-immutable.js index 3555c82f1962f3..5c95213872b635 100644 --- a/test/parallel/test-fs-options-immutable.js +++ b/test/parallel/test-fs-options-immutable.js @@ -70,6 +70,6 @@ if (common.canCreateSymLink()) { { const fileName = path.resolve(tmpdir.path, 'streams'); fs.WriteStream(fileName, options).once('open', common.mustCall(() => { - fs.ReadStream(fileName, options); - })); + fs.ReadStream(fileName, options).destroy(); + })).end(); } diff --git a/test/parallel/test-fs-promises-file-handle-append-file.js b/test/parallel/test-fs-promises-file-handle-append-file.js index f9abef359e0064..90bb6e392516f4 100644 --- a/test/parallel/test-fs-promises-file-handle-append-file.js +++ b/test/parallel/test-fs-promises-file-handle-append-file.js @@ -22,6 +22,8 @@ async function validateAppendBuffer() { await fileHandle.appendFile(buffer); const appendedFileData = fs.readFileSync(filePath); assert.deepStrictEqual(appendedFileData, buffer); + + await fileHandle.close(); } async function validateAppendString() { @@ -33,6 +35,8 @@ async function validateAppendString() { const stringAsBuffer = Buffer.from(string, 'utf8'); const appendedFileData = fs.readFileSync(filePath); assert.deepStrictEqual(appendedFileData, stringAsBuffer); + + await fileHandle.close(); } validateAppendBuffer() diff --git a/test/parallel/test-fs-promises-file-handle-chmod.js b/test/parallel/test-fs-promises-file-handle-chmod.js index 2a05218969ee0a..5a5f84be80ffd9 100644 --- a/test/parallel/test-fs-promises-file-handle-chmod.js +++ b/test/parallel/test-fs-promises-file-handle-chmod.js @@ -38,6 +38,8 @@ async function validateFilePermission() { await fileHandle.chmod(newPermissions); const statsAfterMod = fs.statSync(filePath); assert.deepStrictEqual(statsAfterMod.mode & expectedAccess, expectedAccess); + + await fileHandle.close(); } validateFilePermission().then(common.mustCall()); diff --git a/test/parallel/test-fs-promises-file-handle-read.js b/test/parallel/test-fs-promises-file-handle-read.js index 13f8c277780d07..5ba7fc63e3f44d 100644 --- a/test/parallel/test-fs-promises-file-handle-read.js +++ b/test/parallel/test-fs-promises-file-handle-read.js @@ -26,6 +26,8 @@ async function validateRead() { const readAsyncHandle = await fileHandle.read(Buffer.alloc(11), 0, 11, 0); assert.deepStrictEqual(buffer.length, readAsyncHandle.bytesRead); assert.deepStrictEqual(buffer, readAsyncHandle.buffer); + + await fileHandle.close(); } async function validateEmptyRead() { @@ -38,6 +40,8 @@ async function validateEmptyRead() { fs.closeSync(fd); const readAsyncHandle = await fileHandle.read(Buffer.alloc(11), 0, 11, 0); assert.deepStrictEqual(buffer.length, readAsyncHandle.bytesRead); + + await fileHandle.close(); } async function validateLargeRead() { diff --git a/test/parallel/test-fs-promises-file-handle-readFile.js b/test/parallel/test-fs-promises-file-handle-readFile.js index c31e175019a3fb..4416744bf5189a 100644 --- a/test/parallel/test-fs-promises-file-handle-readFile.js +++ b/test/parallel/test-fs-promises-file-handle-readFile.js @@ -25,6 +25,8 @@ async function validateReadFile() { const readFileData = await fileHandle.readFile(); assert.deepStrictEqual(buffer, readFileData); + + await fileHandle.close(); } async function validateReadFileProc() { diff --git a/test/parallel/test-fs-promises-file-handle-stat.js b/test/parallel/test-fs-promises-file-handle-stat.js index 19ee365213d302..278d3700edc028 100644 --- a/test/parallel/test-fs-promises-file-handle-stat.js +++ b/test/parallel/test-fs-promises-file-handle-stat.js @@ -17,6 +17,7 @@ async function validateStat() { const fileHandle = await open(filePath, 'w+'); const stats = await fileHandle.stat(); assert.ok(stats.mtime instanceof Date); + await fileHandle.close(); } validateStat() diff --git a/test/parallel/test-fs-promises-file-handle-sync.js b/test/parallel/test-fs-promises-file-handle-sync.js index fc6d00c0b8fc13..2c0eca13a73b8c 100644 --- a/test/parallel/test-fs-promises-file-handle-sync.js +++ b/test/parallel/test-fs-promises-file-handle-sync.js @@ -20,6 +20,7 @@ async function validateSync() { const ret = await handle.read(Buffer.alloc(11), 0, 11, 0); assert.strictEqual(ret.bytesRead, 11); assert.deepStrictEqual(ret.buffer, buf); + await handle.close(); } validateSync(); diff --git a/test/parallel/test-fs-promises-file-handle-truncate.js b/test/parallel/test-fs-promises-file-handle-truncate.js index 65f998db748567..ca83755f19e111 100644 --- a/test/parallel/test-fs-promises-file-handle-truncate.js +++ b/test/parallel/test-fs-promises-file-handle-truncate.js @@ -20,6 +20,8 @@ async function validateTruncate() { await fileHandle.truncate(5); assert.deepStrictEqual((await readFile(filename)).toString(), 'Hello'); + + await fileHandle.close(); } validateTruncate().then(common.mustCall()); diff --git a/test/parallel/test-fs-promises-file-handle-write.js b/test/parallel/test-fs-promises-file-handle-write.js index da8cfc0a4bf96b..60840e1e5304d8 100644 --- a/test/parallel/test-fs-promises-file-handle-write.js +++ b/test/parallel/test-fs-promises-file-handle-write.js @@ -22,6 +22,8 @@ async function validateWrite() { await fileHandle.write(buffer, 0, buffer.length); const readFileData = fs.readFileSync(filePathForHandle); assert.deepStrictEqual(buffer, readFileData); + + await fileHandle.close(); } async function validateEmptyWrite() { @@ -32,6 +34,8 @@ async function validateEmptyWrite() { await fileHandle.write(buffer, 0, buffer.length); const readFileData = fs.readFileSync(filePathForHandle); assert.deepStrictEqual(buffer, readFileData); + + await fileHandle.close(); } async function validateNonUint8ArrayWrite() { @@ -42,6 +46,8 @@ async function validateNonUint8ArrayWrite() { await fileHandle.write(buffer, 0, buffer.length); const readFileData = fs.readFileSync(filePathForHandle); assert.deepStrictEqual(Buffer.from(buffer, 'utf8'), readFileData); + + await fileHandle.close(); } async function validateNonStringValuesWrite() { @@ -55,6 +61,8 @@ async function validateNonStringValuesWrite() { const readFileData = fs.readFileSync(filePathForHandle); const expected = ['123', '[object Object]', '[object Map]'].join(''); assert.deepStrictEqual(Buffer.from(expected, 'utf8'), readFileData); + + await fileHandle.close(); } Promise.all([ diff --git a/test/parallel/test-fs-promises-file-handle-writeFile.js b/test/parallel/test-fs-promises-file-handle-writeFile.js index e39c59f5cae80c..25b767a0abcf62 100644 --- a/test/parallel/test-fs-promises-file-handle-writeFile.js +++ b/test/parallel/test-fs-promises-file-handle-writeFile.js @@ -22,6 +22,8 @@ async function validateWriteFile() { await fileHandle.writeFile(buffer); const readFileData = fs.readFileSync(filePathForHandle); assert.deepStrictEqual(buffer, readFileData); + + await fileHandle.close(); } validateWriteFile() diff --git a/test/parallel/test-fs-promises-readfile-with-fd.js b/test/parallel/test-fs-promises-readfile-with-fd.js index 9bf4c185364949..85e4c3fae2b4b6 100644 --- a/test/parallel/test-fs-promises-readfile-with-fd.js +++ b/test/parallel/test-fs-promises-readfile-with-fd.js @@ -28,6 +28,8 @@ async function readFileTest() { /* readFile() should read from position five, instead of zero. */ assert.deepStrictEqual((await handle.readFile()).toString(), ' World'); + + await handle.close(); } diff --git a/test/parallel/test-fs-promises-writefile-with-fd.js b/test/parallel/test-fs-promises-writefile-with-fd.js index a4cc31df7f4d5f..446f9b6d8a1194 100644 --- a/test/parallel/test-fs-promises-writefile-with-fd.js +++ b/test/parallel/test-fs-promises-writefile-with-fd.js @@ -29,6 +29,8 @@ async function writeFileTest() { /* New content should be written at position five, instead of zero. */ assert.deepStrictEqual(readFileSync(fn).toString(), 'HelloWorld'); + + await handle.close(); } diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 4c3d7346ffa996..3c7903c98f04d2 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -83,6 +83,7 @@ async function getHandle(dest) { { const handle = await getHandle(dest); assert.strictEqual(typeof handle, 'object'); + await handle.close(); } // file stats @@ -106,6 +107,7 @@ async function getHandle(dest) { await handle.datasync(); await handle.sync(); + await handle.close(); } // Test fs.read promises when length to read is zero bytes @@ -119,6 +121,7 @@ async function getHandle(dest) { assert.strictEqual(ret.bytesRead, 0); await unlink(dest); + await handle.close(); } // Bytes written to file match buffer @@ -130,6 +133,7 @@ async function getHandle(dest) { const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0); assert.strictEqual(ret.bytesRead, bufLen); assert.deepStrictEqual(ret.buffer, buf); + await handle.close(); } // Truncate file to specified length @@ -143,6 +147,7 @@ async function getHandle(dest) { assert.deepStrictEqual(ret.buffer, buf); await truncate(dest, 5); assert.deepStrictEqual((await readFile(dest)).toString(), 'hello'); + await handle.close(); } // Invalid change of ownership @@ -181,6 +186,8 @@ async function getHandle(dest) { message: 'The value of "gid" is out of range. ' + 'It must be >= 0 && < 4294967296. Received -1' }); + + await handle.close(); } // Set modification times diff --git a/test/parallel/test-fs-readdir-types.js b/test/parallel/test-fs-readdir-types.js index 78f1b0d4e1b627..19e2dba99b60fe 100644 --- a/test/parallel/test-fs-readdir-types.js +++ b/test/parallel/test-fs-readdir-types.js @@ -82,6 +82,7 @@ fs.readdir(readdirDir, { // Check for correct types when the binding returns unknowns const UNKNOWN = constants.UV_DIRENT_UNKNOWN; const oldReaddir = binding.readdir; +process.on('beforeExit', () => { binding.readdir = oldReaddir; }); binding.readdir = common.mustCall((path, encoding, types, req, ctx) => { if (req) { const oldCb = req.oncomplete; diff --git a/test/parallel/test-fs-readfile-fd.js b/test/parallel/test-fs-readfile-fd.js index df0f428bf153c0..10714a91899e69 100644 --- a/test/parallel/test-fs-readfile-fd.js +++ b/test/parallel/test-fs-readfile-fd.js @@ -71,6 +71,8 @@ function tempFdSync(callback) { /* readFileSync() should read from position five, instead of zero. */ assert.deepStrictEqual(fs.readFileSync(fd).toString(), ' World'); + + fs.closeSync(fd); } { @@ -89,6 +91,8 @@ function tempFdSync(callback) { assert.ifError(err); /* readFile() should read from position five, instead of zero. */ assert.deepStrictEqual(data.toString(), ' World'); + + fs.closeSync(fd); })); })); })); diff --git a/test/parallel/test-fs-readfile-pipe-large.js b/test/parallel/test-fs-readfile-pipe-large.js index 78c5feedb7210c..a32e79833482a5 100644 --- a/test/parallel/test-fs-readfile-pipe-large.js +++ b/test/parallel/test-fs-readfile-pipe-large.js @@ -43,7 +43,3 @@ exec(cmd, { maxBuffer: 1000000 }, common.mustCall((err, stdout, stderr) => { ); console.log('ok'); })); - -process.on('exit', function() { - fs.unlinkSync(filename); -}); diff --git a/test/parallel/test-fs-readfilesync-pipe-large.js b/test/parallel/test-fs-readfilesync-pipe-large.js index 0f180f0f86005c..c8ea474ea7668e 100644 --- a/test/parallel/test-fs-readfilesync-pipe-large.js +++ b/test/parallel/test-fs-readfilesync-pipe-large.js @@ -36,7 +36,3 @@ exec( console.log('ok'); }) ); - -process.on('exit', function() { - fs.unlinkSync(filename); -}); diff --git a/test/parallel/test-fs-ready-event-stream.js b/test/parallel/test-fs-ready-event-stream.js index f6b380cf26d42c..68e8fb76b771f4 100644 --- a/test/parallel/test-fs-ready-event-stream.js +++ b/test/parallel/test-fs-ready-event-stream.js @@ -17,4 +17,5 @@ const writeStream = fs.createWriteStream(writeFile, { autoClose: true }); assert.strictEqual(writeStream.pending, true); writeStream.on('ready', common.mustCall(() => { assert.strictEqual(writeStream.pending, false); + writeStream.end(); })); diff --git a/test/parallel/test-fs-truncate-fd.js b/test/parallel/test-fs-truncate-fd.js index f71d511f5da057..057e4cb58df705 100644 --- a/test/parallel/test-fs-truncate-fd.js +++ b/test/parallel/test-fs-truncate-fd.js @@ -21,7 +21,7 @@ fs.truncate(fd, 5, common.mustCall((err) => { assert.strictEqual(fs.readFileSync(filename, 'utf8'), 'hello'); })); -process.on('exit', () => { +process.on('beforeExit', () => { fs.closeSync(fd); fs.unlinkSync(filename); console.log('ok'); diff --git a/test/parallel/test-fs-truncate.js b/test/parallel/test-fs-truncate.js index 95036dc9f52195..044ff622bdb129 100644 --- a/test/parallel/test-fs-truncate.js +++ b/test/parallel/test-fs-truncate.js @@ -147,7 +147,7 @@ function testFtruncate(cb) { const file2 = path.resolve(tmp, 'truncate-file-2.txt'); fs.writeFileSync(file2, 'Hi'); const fd = fs.openSync(file2, 'r+'); - process.on('exit', () => fs.closeSync(fd)); + process.on('beforeExit', () => fs.closeSync(fd)); fs.ftruncateSync(fd, 4); assert(fs.readFileSync(file2).equals(Buffer.from('Hi\u0000\u0000'))); } @@ -165,7 +165,7 @@ function testFtruncate(cb) { const file4 = path.resolve(tmp, 'truncate-file-4.txt'); fs.writeFileSync(file4, 'Hi'); const fd = fs.openSync(file4, 'r+'); - process.on('exit', () => fs.closeSync(fd)); + process.on('beforeExit', () => fs.closeSync(fd)); fs.ftruncate(fd, 4, common.mustCall(function(err) { assert.ifError(err); assert(fs.readFileSync(file4).equals(Buffer.from('Hi\u0000\u0000'))); @@ -176,7 +176,7 @@ function testFtruncate(cb) { const file5 = path.resolve(tmp, 'truncate-file-5.txt'); fs.writeFileSync(file5, 'Hi'); const fd = fs.openSync(file5, 'r+'); - process.on('exit', () => fs.closeSync(fd)); + process.on('beforeExit', () => fs.closeSync(fd)); ['', false, null, {}, []].forEach((input) => { assert.throws( @@ -232,7 +232,7 @@ function testFtruncate(cb) { const file6 = path.resolve(tmp, 'truncate-file-6.txt'); fs.writeFileSync(file6, 'Hi'); const fd = fs.openSync(file6, 'r+'); - process.on('exit', () => fs.closeSync(fd)); + process.on('beforeExit', () => fs.closeSync(fd)); fs.ftruncate(fd, -1, common.mustCall(function(err) { assert.ifError(err); assert(fs.readFileSync(file6).equals(Buffer.from(''))); diff --git a/test/parallel/test-fs-write-file.js b/test/parallel/test-fs-write-file.js index b9536bea4ca131..97f29c081bcd7c 100644 --- a/test/parallel/test-fs-write-file.js +++ b/test/parallel/test-fs-write-file.js @@ -102,10 +102,3 @@ fs.open(filename4, 'w+', common.mustCall(function(e, fd) { })); })); })); - -process.on('exit', function() { - fs.unlinkSync(filename); - fs.unlinkSync(filename2); - fs.unlinkSync(filename3); - fs.unlinkSync(filename4); -}); diff --git a/test/parallel/test-fs-write-stream-change-open.js b/test/parallel/test-fs-write-stream-change-open.js index 56dbafd656431b..2fc06cf901615b 100644 --- a/test/parallel/test-fs-write-stream-change-open.js +++ b/test/parallel/test-fs-write-stream-change-open.js @@ -46,6 +46,7 @@ fs.close = function(fd) { assert.ok(fd, 'fs.close must not be called with an undefined fd.'); fs.close = _fs_close; fs.open = _fs_open; + fs.closeSync(fd); }; stream.write('foo'); diff --git a/test/parallel/test-fs-write-stream-err.js b/test/parallel/test-fs-write-stream-err.js index 0db517da2a58d9..6d6f25b21f0e27 100644 --- a/test/parallel/test-fs-write-stream-err.js +++ b/test/parallel/test-fs-write-stream-err.js @@ -56,6 +56,7 @@ fs.write = function() { fs.close = common.mustCall(function(fd_, cb) { console.error('fs.close', fd_, stream.fd); assert.strictEqual(fd_, stream.fd); + fs.closeSync(fd_); process.nextTick(cb); }); diff --git a/test/parallel/test-fs-write-stream-throw-type-error.js b/test/parallel/test-fs-write-stream-throw-type-error.js index c80f647b3d622e..9a3d1cf16dd986 100644 --- a/test/parallel/test-fs-write-stream-throw-type-error.js +++ b/test/parallel/test-fs-write-stream-throw-type-error.js @@ -9,10 +9,10 @@ const example = path.join(tmpdir.path, 'dummy'); tmpdir.refresh(); // Should not throw. -fs.createWriteStream(example, undefined); -fs.createWriteStream(example, null); -fs.createWriteStream(example, 'utf8'); -fs.createWriteStream(example, { encoding: 'utf8' }); +fs.createWriteStream(example, undefined).end(); +fs.createWriteStream(example, null).end(); +fs.createWriteStream(example, 'utf8').end(); +fs.createWriteStream(example, { encoding: 'utf8' }).end(); const createWriteStreamErr = (path, opt) => { common.expectsError( diff --git a/test/parallel/test-fs-write-stream.js b/test/parallel/test-fs-write-stream.js index e93f65e604c20b..5e75f4a0b306d2 100644 --- a/test/parallel/test-fs-write-stream.js +++ b/test/parallel/test-fs-write-stream.js @@ -38,6 +38,7 @@ tmpdir.refresh(); fs.close = function(fd) { assert.ok(fd, 'fs.close must not be called without an undefined fd.'); fs.close = _fs_close; + fs.closeSync(fd); }; stream.destroy(); } diff --git a/test/parallel/test-fs-writefile-with-fd.js b/test/parallel/test-fs-writefile-with-fd.js index 6851518d4dcc9a..a3436006b46a0e 100644 --- a/test/parallel/test-fs-writefile-with-fd.js +++ b/test/parallel/test-fs-writefile-with-fd.js @@ -30,6 +30,9 @@ tmpdir.refresh(); /* New content should be written at position five, instead of zero. */ assert.deepStrictEqual(fs.readFileSync(filename).toString(), 'HelloWorld'); + + /* Close the file descriptor. */ + fs.closeSync(fd); } { @@ -52,6 +55,9 @@ tmpdir.refresh(); /* New content should be written at position five, instead of zero. */ assert.deepStrictEqual(fs.readFileSync(file).toString(), 'HelloWorld'); + + /* Close the file descriptor. */ + fs.closeSync(fd); })); })); })); diff --git a/test/parallel/test-http2-respond-file-error-pipe-offset.js b/test/parallel/test-http2-respond-file-error-pipe-offset.js index eb782e2dea66c4..1f3b7163f25ea1 100644 --- a/test/parallel/test-http2-respond-file-error-pipe-offset.js +++ b/test/parallel/test-http2-respond-file-error-pipe-offset.js @@ -21,8 +21,6 @@ if (mkfifo.error && mkfifo.error.code === 'ENOENT') { common.skip('missing mkfifo'); } -process.on('exit', () => fs.unlinkSync(pipeName)); - const server = http2.createServer(); server.on('stream', (stream) => { stream.respondWithFile(pipeName, { diff --git a/test/parallel/test-http2-respond-file-with-pipe.js b/test/parallel/test-http2-respond-file-with-pipe.js index 2b7ca3fc9a51ee..d0557c2fd01e3a 100644 --- a/test/parallel/test-http2-respond-file-with-pipe.js +++ b/test/parallel/test-http2-respond-file-with-pipe.js @@ -21,8 +21,6 @@ if (mkfifo.error && mkfifo.error.code === 'ENOENT') { common.skip('missing mkfifo'); } -process.on('exit', () => fs.unlinkSync(pipeName)); - const server = http2.createServer(); server.on('stream', (stream) => { stream.respondWithFile(pipeName, { diff --git a/test/parallel/test-internal-fs-syncwritestream.js b/test/parallel/test-internal-fs-syncwritestream.js index bf4776550b6edf..383b70512c7e53 100644 --- a/test/parallel/test-internal-fs-syncwritestream.js +++ b/test/parallel/test-internal-fs-syncwritestream.js @@ -38,6 +38,8 @@ const filename = path.join(tmpdir.path, 'sync-write-stream.txt'); assert.strictEqual(stream._write(chunk, null, common.mustCall(1)), true); assert.strictEqual(fs.readFileSync(filename).equals(chunk), true); + + fs.closeSync(fd); } // Verify that the stream will unset the fd after destroy(). diff --git a/test/parallel/test-readline-async-iterators-destroy.js b/test/parallel/test-readline-async-iterators-destroy.js index 746981a1ae7cfd..ea174d51723aa8 100644 --- a/test/parallel/test-readline-async-iterators-destroy.js +++ b/test/parallel/test-readline-async-iterators-destroy.js @@ -42,6 +42,9 @@ async function testSimpleDestroy() { expectedLines.splice(1); assert.deepStrictEqual(iteratedLines, expectedLines); + + rli.close(); + readable.destroy(); } } @@ -72,6 +75,9 @@ async function testMutualDestroy() { } assert.deepStrictEqual(iteratedLines, expectedLines); + + rli.close(); + readable.destroy(); } } diff --git a/test/sequential/test-http2-timeout-large-write-file.js b/test/sequential/test-http2-timeout-large-write-file.js index bb366cfff04091..520958bd57f6d4 100644 --- a/test/sequential/test-http2-timeout-large-write-file.js +++ b/test/sequential/test-http2-timeout-large-write-file.js @@ -33,6 +33,7 @@ const content = Buffer.alloc(writeSize, 0x44); const filepath = path.join(tmpdir.path, 'http2-large-write.tmp'); fs.writeFileSync(filepath, content, 'binary'); const fd = fs.openSync(filepath, 'r'); +process.on('beforeExit', () => fs.closeSync(fd)); const server = http2.createSecureServer({ key: fixtures.readKey('agent1-key.pem'), From 91b6d559d0bd80555e4ae05f2c5189e5753e297f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 14:53:21 +0100 Subject: [PATCH 06/11] fixup! test: clean tmpdir on process exit Suggestions by Rich Trott --- test/common/README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/common/README.md b/test/common/README.md index 3ad21ea4d0cc3d..76ab605c79ee39 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -906,11 +906,12 @@ The realpath of the testing temporary directory. Deletes and recreates the testing temporary directory. -The first time refresh is run it adds a listener to process `'exit'` that +The first time `refresh()` runs, it adds a listener to process `'exit'` that cleans the temporary directory. Thus, every file under `tmpdir.path` needs to be closed before the test completes. A good way to do this is to add a -listener to process `'beforeExit'`. If a file needs to be left open until Node -completes, use a child process and call `refresh` only in the parent. +listener to process `'beforeExit'`. If a file needs to be left open until +Node.js completes, use a child process and call `refresh()` only in the +parent. ## WPT Module From d68f3161a277b33bf1f0cf3cd50f6368ce99f162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 15:14:12 +0100 Subject: [PATCH 07/11] fixup! test: clean tmpdir on process exit Fixes for workers --- test/common/tmpdir.js | 4 +++- test/parallel/test-fs-truncate-fd.js | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/common/tmpdir.js b/test/common/tmpdir.js index a71f5e95155ab8..ca82e7bca0e7b3 100644 --- a/test/common/tmpdir.js +++ b/test/common/tmpdir.js @@ -5,6 +5,7 @@ const { execSync } = require('child_process'); const fs = require('fs'); const path = require('path'); const { debuglog } = require('util'); +const { isMainThread } = require('worker_threads'); const debug = debuglog('test/tmpdir'); @@ -110,7 +111,8 @@ function refresh(opts = {}) { process.on('exit', () => { try { // Change dit to avoid possible EBUSY - process.chdir(testRoot); + if (isMainThread) + process.chdir(testRoot); rimrafSync(tmpPath, { spawn: false }); } catch (e) { console.error('Can\'t clean tmpdir:', tmpPath); diff --git a/test/parallel/test-fs-truncate-fd.js b/test/parallel/test-fs-truncate-fd.js index 057e4cb58df705..fbddbb32d5c0fc 100644 --- a/test/parallel/test-fs-truncate-fd.js +++ b/test/parallel/test-fs-truncate-fd.js @@ -21,7 +21,7 @@ fs.truncate(fd, 5, common.mustCall((err) => { assert.strictEqual(fs.readFileSync(filename, 'utf8'), 'hello'); })); -process.on('beforeExit', () => { +process.once('beforeExit', () => { fs.closeSync(fd); fs.unlinkSync(filename); console.log('ok'); From 2423e419ca47895eb634e0e680e0adbd20fddccf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 30 Jul 2019 16:04:59 +0100 Subject: [PATCH 08/11] fixup! test: clean tmpdir on process exit Fixes for pummel tests --- test/pummel/test-fs-largefile.js | 6 ------ test/pummel/test-fs-watch-file-slow.js | 1 + test/pummel/test-fs-watch-file.js | 5 +---- test/pummel/test-regress-GH-814.js | 2 ++ test/pummel/test-regress-GH-814_2.js | 2 ++ 5 files changed, 6 insertions(+), 10 deletions(-) diff --git a/test/pummel/test-fs-largefile.js b/test/pummel/test-fs-largefile.js index 8200543b9a13e7..409d7a2112f027 100644 --- a/test/pummel/test-fs-largefile.js +++ b/test/pummel/test-fs-largefile.js @@ -47,9 +47,3 @@ assert.strictEqual(readBuf[0], 0); // Verify that floating point positions do not throw. fs.writeSync(fd, writeBuf, 0, writeBuf.length, 42.000001); fs.close(fd, common.mustCall()); - -// Normally, we don't clean up tmp files at the end of a test, but we'll make an -// exception for a 5 GB file. -process.on('exit', function() { - fs.unlinkSync(filepath); -}); diff --git a/test/pummel/test-fs-watch-file-slow.js b/test/pummel/test-fs-watch-file-slow.js index 94f2a263881427..c7513a18e6fa3e 100644 --- a/test/pummel/test-fs-watch-file-slow.js +++ b/test/pummel/test-fs-watch-file-slow.js @@ -27,6 +27,7 @@ const fs = require('fs'); const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); const FILENAME = path.join(tmpdir.path, 'watch-me'); const TIMEOUT = 1300; diff --git a/test/pummel/test-fs-watch-file.js b/test/pummel/test-fs-watch-file.js index 7a8854860c15f1..0d897bdc4adf94 100644 --- a/test/pummel/test-fs-watch-file.js +++ b/test/pummel/test-fs-watch-file.js @@ -46,10 +46,6 @@ const filenameThree = 'charm'; // Because the third time is const filenameFour = 'get'; process.on('exit', function() { - fs.unlinkSync(filepathOne); - fs.unlinkSync(filepathTwoAbs); - fs.unlinkSync(filenameThree); - fs.unlinkSync(filenameFour); assert.strictEqual(watchSeenOne, 1); assert.strictEqual(watchSeenTwo, 2); assert.strictEqual(watchSeenThree, 1); @@ -57,6 +53,7 @@ process.on('exit', function() { }); +tmpdir.refresh(); fs.writeFileSync(filepathOne, 'hello'); assert.throws( diff --git a/test/pummel/test-regress-GH-814.js b/test/pummel/test-regress-GH-814.js index d7af574e0ab87e..3ae6dd061c91df 100644 --- a/test/pummel/test-regress-GH-814.js +++ b/test/pummel/test-regress-GH-814.js @@ -37,6 +37,8 @@ function newBuffer(size, value) { } const fs = require('fs'); + +tmpdir.refresh(); const testFileName = require('path').join(tmpdir.path, 'GH-814_testFile.txt'); const testFileFD = fs.openSync(testFileName, 'w'); console.log(testFileName); diff --git a/test/pummel/test-regress-GH-814_2.js b/test/pummel/test-regress-GH-814_2.js index 1b40ca7b37cdfa..10de38a50c13fd 100644 --- a/test/pummel/test-regress-GH-814_2.js +++ b/test/pummel/test-regress-GH-814_2.js @@ -27,6 +27,8 @@ const assert = require('assert'); const fs = require('fs'); const tmpdir = require('../common/tmpdir'); + +tmpdir.refresh(); const testFileName = require('path').join(tmpdir.path, 'GH-814_test.txt'); const testFD = fs.openSync(testFileName, 'w'); console.error(`${testFileName}\n`); From debcba4e3ccc852fdc91f6e8d35a5fe0af1260cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Wed, 31 Jul 2019 20:42:45 +0100 Subject: [PATCH 09/11] fixup! test: clean tmpdir on process exit Warn about NFS files --- test/common/tmpdir.js | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/test/common/tmpdir.js b/test/common/tmpdir.js index ca82e7bca0e7b3..c66756e21b47f2 100644 --- a/test/common/tmpdir.js +++ b/test/common/tmpdir.js @@ -108,18 +108,32 @@ function refresh(opts = {}) { firstRefresh = false; // Clean only when a test uses refresh. This allows for child processes to // use the tmpdir and only the parent will clean on exit. - process.on('exit', () => { - try { - // Change dit to avoid possible EBUSY - if (isMainThread) - process.chdir(testRoot); - rimrafSync(tmpPath, { spawn: false }); - } catch (e) { - console.error('Can\'t clean tmpdir:', tmpPath); - console.error('Files blocking:', fs.readdirSync(tmpPath)); - throw e; - } - }); + process.on('exit', onexit); + } +} + +function onexit() { + // Change directory to avoid possible EBUSY + if (isMainThread) + process.chdir(testRoot); + + try { + rimrafSync(tmpPath, { spawn: false }); + } catch (e) { + console.error('Can\'t clean tmpdir:', tmpPath); + + const files = fs.readdirSync(tmpPath); + console.error('Files blocking:', files); + + if (files.some((f) => f.startsWith('.nfs'))) { + // Warn about NFS "silly rename" + console.error('Note: ".nfs*" might be files that were open and ' + + 'unlinked but not closed.'); + console.error('See http://nfs.sourceforge.net/#faq_d2 for details.'); + } + + console.error(); + throw e; } } From b3392451aa68aea2e106cf9179929655f3a3bf87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Wed, 31 Jul 2019 21:57:10 +0100 Subject: [PATCH 10/11] fixup! test: clean tmpdir on process exit Fixes for Linux open files --- test/parallel/test-fs-open-flags.js | 5 ++++- test/parallel/test-pipe-unref.js | 19 ++++++++++++++++++- test/parallel/test-repl-history-perm.js | 5 ++++- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-fs-open-flags.js b/test/parallel/test-fs-open-flags.js index d1fee24de690ac..62866cfb1b251d 100644 --- a/test/parallel/test-fs-open-flags.js +++ b/test/parallel/test-fs-open-flags.js @@ -94,5 +94,8 @@ if (common.isLinux || common.isOSX) { tmpdir.refresh(); const file = path.join(tmpdir.path, 'a.js'); fs.copyFileSync(fixtures.path('a.js'), file); - fs.open(file, O_DSYNC, common.mustCall(assert.ifError)); + fs.open(file, O_DSYNC, common.mustCall((err, fd) => { + assert.ifError(err); + fs.closeSync(fd); + })); } diff --git a/test/parallel/test-pipe-unref.js b/test/parallel/test-pipe-unref.js index 1e0245b5444f62..78419a1d77076e 100644 --- a/test/parallel/test-pipe-unref.js +++ b/test/parallel/test-pipe-unref.js @@ -1,12 +1,29 @@ 'use strict'; const common = require('../common'); const net = require('net'); +const assert = require('assert'); +const { fork } = require('child_process'); // This test should end immediately after `unref` is called +// The pipe will stay open as Node.js completes, thus run in a child process +// so that tmpdir can be cleaned up. const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); +if (process.argv[2] !== 'child') { + // Parent + tmpdir.refresh(); + + // Run test + const child = fork(__filename, ['child'], { stdio: 'inherit' }); + child.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); + + return; +} + +// Child const s = net.Server(); s.listen(common.PIPE); s.unref(); diff --git a/test/parallel/test-repl-history-perm.js b/test/parallel/test-repl-history-perm.js index 03ce1435d9ba4e..3db2a009eb053b 100644 --- a/test/parallel/test-repl-history-perm.js +++ b/test/parallel/test-repl-history-perm.js @@ -38,12 +38,15 @@ const replHistoryPath = path.join(tmpdir.path, '.node_repl_history'); const checkResults = common.mustCall(function(err, r) { assert.ifError(err); - r.input.end(); const stat = fs.statSync(replHistoryPath); const fileMode = stat.mode & 0o777; assert.strictEqual( fileMode, 0o600, `REPL history file should be mode 0600 but was 0${fileMode.toString(8)}`); + + // Close the REPL + r.input.emit('keypress', '', { ctrl: true, name: 'd' }); + r.input.end(); }); repl.createInternalRepl( From 3c678789a5736c9684b2c77acb2564111311cb09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Fri, 2 Aug 2019 23:23:42 +0100 Subject: [PATCH 11/11] fixup! test: clean tmpdir on process exit --- .../test-child-process-server-close.js | 21 +++++++++++++++++-- .../parallel/test-tls-wrap-econnreset-pipe.js | 19 ++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-child-process-server-close.js b/test/parallel/test-child-process-server-close.js index d70926f2e8278e..8ad32b4bbfcbd9 100644 --- a/test/parallel/test-child-process-server-close.js +++ b/test/parallel/test-child-process-server-close.js @@ -1,12 +1,29 @@ 'use strict'; const common = require('../common'); -const { spawn } = require('child_process'); +const assert = require('assert'); +const { fork, spawn } = require('child_process'); const net = require('net'); const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); +// Run in a child process because the PIPE file descriptor stays open until +// Node.js completes, blocking the tmpdir and preventing cleanup. + +if (process.argv[2] !== 'child') { + // Parent + tmpdir.refresh(); + + // Run test + const child = fork(__filename, ['child'], { stdio: 'inherit' }); + child.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); + + return; +} + +// Child const server = net.createServer((conn) => { spawn(process.execPath, ['-v'], { stdio: ['ignore', conn, 'ignore'] diff --git a/test/parallel/test-tls-wrap-econnreset-pipe.js b/test/parallel/test-tls-wrap-econnreset-pipe.js index b400e35d412392..a5cba7e47bfd06 100644 --- a/test/parallel/test-tls-wrap-econnreset-pipe.js +++ b/test/parallel/test-tls-wrap-econnreset-pipe.js @@ -7,10 +7,27 @@ if (!common.hasCrypto) const assert = require('assert'); const tls = require('tls'); const net = require('net'); +const { fork } = require('child_process'); const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); +// Run in a child process because the PIPE file descriptor stays open until +// Node.js completes, blocking the tmpdir and preventing cleanup. + +if (process.argv[2] !== 'child') { + // Parent + tmpdir.refresh(); + + // Run test + const child = fork(__filename, ['child'], { stdio: 'inherit' }); + child.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); + + return; +} + +// Child const server = net.createServer((c) => { c.end(); }).listen(common.PIPE, common.mustCall(() => {