From 421b57183d94fd60ce6c1e225c1e19039d4335ce Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 18 Oct 2015 14:42:44 -0700 Subject: [PATCH 1/5] test: fix flaky test-child-process-emfile Require the test setup to obtain an EMFILE error and not ENFILE as ENFILE means there is a race condition with other processes that may close files before `spawn()` is called by the test. Fixes: https://github.com/nodejs/node/issues/2666 --- test/sequential/test-child-process-emfile.js | 35 +++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/test/sequential/test-child-process-emfile.js b/test/sequential/test-child-process-emfile.js index ae7964d9e7a7d6..4f2cbdc9068e2a 100644 --- a/test/sequential/test-child-process-emfile.js +++ b/test/sequential/test-child-process-emfile.js @@ -1,30 +1,49 @@ 'use strict'; -var common = require('../common'); -var assert = require('assert'); -var spawn = require('child_process').spawn; -var fs = require('fs'); +const common = require('../common'); +const assert = require('assert'); +const child_process = require('child_process'); +const fs = require('fs'); if (common.isWindows) { console.log('1..0 # Skipped: no RLIMIT_NOFILE on Windows'); return; } -var openFds = []; +const ulimit = Number(child_process.execSync('ulimit -n')); +if (ulimit > 64) { + // Sorry about this nonsense. It can be replaced if + // https://github.com/nodejs/node-v0.x-archive/pull/2143#issuecomment-2847886 + // ever happens. + const result = child_process.spawnSync( + '/bin/sh', + ['-c', `ulimit -n 64 && ${process.execPath} ${__filename}`] + ); + assert.strictEqual(result.stdout.toString(), ''); + assert.strictEqual(result.stderr.toString(), ''); + assert.strictEqual(result.status, 0); + assert.strictEqual(result.error, undefined); + return; +} + +const openFds = []; for (;;) { try { openFds.push(fs.openSync(__filename, 'r')); } catch (err) { - assert(err.code === 'EMFILE' || err.code === 'ENFILE'); + if (err.code === 'ENFILE') { + continue; + } + assert(err.code === 'EMFILE'); break; } } // Should emit an error, not throw. -var proc = spawn(process.execPath, ['-e', '0']); +const proc = child_process.spawn(process.execPath, ['-e', '0']); proc.on('error', common.mustCall(function(err) { - assert(err.code === 'EMFILE' || err.code === 'ENFILE'); + assert.strictEqual(err.code, 'EMFILE'); })); proc.on('exit', function() { From 30edd30c90cd960525bc75e7ae06861e9a46813b Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 18 Oct 2015 20:56:29 -0700 Subject: [PATCH 2/5] fixup --- test/sequential/test-child-process-emfile.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/sequential/test-child-process-emfile.js b/test/sequential/test-child-process-emfile.js index 4f2cbdc9068e2a..b9b741d56ea5ba 100644 --- a/test/sequential/test-child-process-emfile.js +++ b/test/sequential/test-child-process-emfile.js @@ -31,9 +31,6 @@ for (;;) { try { openFds.push(fs.openSync(__filename, 'r')); } catch (err) { - if (err.code === 'ENFILE') { - continue; - } assert(err.code === 'EMFILE'); break; } From 55561960833070ada7377efd7afcdd3fbfa46deb Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 19 Oct 2015 21:29:37 -0700 Subject: [PATCH 3/5] fixup: handle `unlimited` being coerced to NaN --- test/sequential/test-child-process-emfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-child-process-emfile.js b/test/sequential/test-child-process-emfile.js index b9b741d56ea5ba..f3f3cea34b332c 100644 --- a/test/sequential/test-child-process-emfile.js +++ b/test/sequential/test-child-process-emfile.js @@ -10,7 +10,7 @@ if (common.isWindows) { } const ulimit = Number(child_process.execSync('ulimit -n')); -if (ulimit > 64) { +if (ulimit > 64 || Number.isNaN(ulimit)) { // Sorry about this nonsense. It can be replaced if // https://github.com/nodejs/node-v0.x-archive/pull/2143#issuecomment-2847886 // ever happens. From 1a6d2e6890e2555f7a8ca3bc2a06ad0979268105 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 20 Oct 2015 09:44:03 -0700 Subject: [PATCH 4/5] fixup: set with -n, check with -Hn --- test/sequential/test-child-process-emfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-child-process-emfile.js b/test/sequential/test-child-process-emfile.js index f3f3cea34b332c..89bb98d5b157f9 100644 --- a/test/sequential/test-child-process-emfile.js +++ b/test/sequential/test-child-process-emfile.js @@ -9,7 +9,7 @@ if (common.isWindows) { return; } -const ulimit = Number(child_process.execSync('ulimit -n')); +const ulimit = Number(child_process.execSync('ulimit -Hn')); if (ulimit > 64 || Number.isNaN(ulimit)) { // Sorry about this nonsense. It can be replaced if // https://github.com/nodejs/node-v0.x-archive/pull/2143#issuecomment-2847886 From 5f5bb3639c99fd8868da8a798d7930210589b4eb Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 20 Oct 2015 09:47:50 -0700 Subject: [PATCH 5/5] fixup: add quotes around paths --- test/sequential/test-child-process-emfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-child-process-emfile.js b/test/sequential/test-child-process-emfile.js index 89bb98d5b157f9..2ac0b6c0b23a73 100644 --- a/test/sequential/test-child-process-emfile.js +++ b/test/sequential/test-child-process-emfile.js @@ -16,7 +16,7 @@ if (ulimit > 64 || Number.isNaN(ulimit)) { // ever happens. const result = child_process.spawnSync( '/bin/sh', - ['-c', `ulimit -n 64 && ${process.execPath} ${__filename}`] + ['-c', `ulimit -n 64 && '${process.execPath}' '${__filename}'`] ); assert.strictEqual(result.stdout.toString(), ''); assert.strictEqual(result.stderr.toString(), '');