Fix flaky test-child-process-emfile #3430
Conversation
|
CI: https://ci.nodejs.org/job/node-test-pull-request/531/ EDIT: Pushed a commit to remove some cruft that wasn't needed. New CI: https://ci.nodejs.org/job/node-test-pull-request/532/ |
mscdex
added
the
child_process
|
|
||
| if (common.isWindows) { | ||
| console.log('1..0 # Skipped: no RLIMIT_NOFILE on Windows'); | ||
| return; | ||
| } | ||
|
|
||
| var openFds = []; | ||
| const ulimit = Number(child_process.execSync('ulimit -n')); |
There was a problem hiding this comment.
You should probably exec ulimit -Hn here because node sets RLIMIT_NOFILE to the hard limit at startup. I'm not sure how portable the -H flag is though.
There was a problem hiding this comment.
Use of -H requires super-user privileges, at least on OS X. This means make test will fail for people unless they run it as root.
There was a problem hiding this comment.
Even when just querying the hard limit? That seems to work on my 10.8 macbook, FWIW.
That did remind me of something: ulimit can print "unlimited". That gets coerced to NaN and will fail the ulimit > 64 check.
There was a problem hiding this comment.
Re portability: ulimit -Hn works for me without root privileges on our smartos and freebsd boxes (also works locally on osx 10.11).
There was a problem hiding this comment.
ulimit -Hn works fine on OS X without super-user privileges. It's setting (e.g., ulimit -Hn 64) that requires super-user.
Good catch on the unlimited string. I'll fix that...
There was a problem hiding this comment.
@jbergstroem I guess I can go and run it on CI and see what happens...
There was a problem hiding this comment.
@Trott breaks on setting smartos though: -bash: ulimit: open files: cannot modify limit: Invalid argument
There was a problem hiding this comment.
I'd use ulimit -Hn for querying, ulimit -n num is fine for setting it because it sets both the hard and soft limit. Just querying for ulimit -n would be wrong in the (admittedly unlikely) case that the soft limit is <= 64 but the hard limit is > 64.
There was a problem hiding this comment.
@jbergstroem ulimit -Hn <integer> works without super-user privs for me if and only if I first set the soft limit as low or lower than what I'm trying to set the hard limit to. (In retrospect: Duh!) And sudo doesn't fix it. I thought it did because I was testing in different shell instances and must have had the soft limit already set appropriately in one of them.
TL;DR: You were right to be surprised (skeptical?) of my results.
The @bnoordhuis plan of setting with -n and querying with -Hn makes the most sense to me.
There was a problem hiding this comment.
P.S. The error message you got on smartos is what I was getting on OS X so maybe check that you weren't trying to set the hard limit lower than the soft limit? (Although it doesn't really matter since we can and, in this case, should set the soft limit as well so we can just use -n.)
|
LGTM with a comment. |
|
@jasnell LTS? |
Trott
force-pushed
the
flaky-test-child-process-emfile
branch
from
3ce3dba
to
114d317
Compare
|
Added handling for New CI: |
|
..that'd be my bad. Was doing some changes to the |
| // ever happens. | ||
| const result = child_process.spawnSync( | ||
| '/bin/sh', | ||
| ['-c', `ulimit -n 64 && ${process.execPath} ${__filename}`] |
There was a problem hiding this comment.
Consider putting quotes around the paths, in case there are spaces in them.
There was a problem hiding this comment.
Ah, yeah, good one. Thanks. Done. There's probably still edge cases (quotation marks in the path?) but this should deal with all the most likely ones. Thanks again.
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: nodejs#2666
Trott
force-pushed
the
flaky-test-child-process-emfile
branch
from
114d317
to
1a6d2e6
Compare
|
CI with latest @bnoordhuis suggestions: https://ci.nodejs.org/job/node-test-commit/892/ |
|
LGTM |
|
LGTM. @Trott can I ask you to squash the commits down please? |
|
@jasnell Yup. I always squash 'em before I land. |
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: nodejs#2666 PR-URL: nodejs#3430 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in d89bc7b. |
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: #2666 PR-URL: #3430 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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: #2666 PR-URL: #3430 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in v4.x-staging in 0b32414 |
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: #2666 PR-URL: #3430 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
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.This test as is had failed 6 out of 26 times that are in the Jenkins history for freebsd101-32 at https://ci.nodejs.org/job/node-test-commit-freebsd/. With the fix, it has run successfully 12 out of 12 times. (UPDATE: 14 out of 14 now.)
Fixes: #2666