Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: improve flaky test-listen-fd-ebadf.js #17797

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 20, 2017

Find an invalid file descriptor rather than assuming 42 will be invalid.

Fixes: #17762

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test net

@Trott Trott added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Dec 20, 2017
@Trott
Copy link
Member Author

Trott commented Dec 20, 2017

Ping @gibfahn

@Trott
Copy link
Member Author

Trott commented Dec 20, 2017

@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Seems reasonable, let me try it on the machine that was failing.

@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

descripter -> descriptor in the commit message btw

Find an invalid file descriptor rather than assuming 42 will be invalid.

Fixes: nodejs#17762
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

$ tools/test.py -J --repeat 1000 parallel/test-listen-fd-ebadf
Command: out/Release/node /dev/shm/gib/node/test/parallel/test-listen-fd-ebadf.js
[00:08|% 100|+ 980|-  20]: Done                        
$ tools/test.py -J --repeat 1000 parallel/test-listen-fd-ebadf-trott
[00:08|% 100|+ 1000|-   0]: Done

Looks like that fixes it, thanks @Trott !

EDIT: Since the machine is idle, why not do a proper stress test:

$ tools/test.py -J --repeat 100000 parallel/test-listen-fd-ebadf-trott
[25:31|% 100|+ 100000|-   0]: Done

@Trott
Copy link
Member Author

Trott commented Dec 21, 2017

(CI issues are infra-related, unrelated to this PR.)

@Trott
Copy link
Member Author

Trott commented Dec 21, 2017

Landed in 1d87891

@Trott Trott closed this Dec 21, 2017
Trott added a commit to Trott/io.js that referenced this pull request Dec 21, 2017
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: nodejs#17797
Fixes: nodejs#17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gibfahn
Copy link
Member

gibfahn commented Jan 24, 2018

Doesn't land cleanly on 6.x. Assuming this test is flaky there too, a backport would be useful.

Diff:

diff --git a/test/parallel/test-listen-fd-ebadf.js b/test/parallel/test-listen-fd-ebadf.js
index dfa99e274a..8153803e30 100644
--- a/test/parallel/test-listen-fd-ebadf.js
+++ b/test/parallel/test-listen-fd-ebadf.js
@@ -1,11 +1,29 @@
 'use strict';
 const common = require('../common');
+
 const assert = require('assert');
+const fs = require('fs');
 const net = require('net');
 
 net.createServer(common.mustNotCall()).listen({fd: 2})
   .on('error', common.mustCall(onError));
+<<<<<<< HEAD
 net.createServer(common.mustNotCall()).listen({fd: 42})
+||||||| parent of 1d8789188f... test: improve flaky test-listen-fd-ebadf.js
+net.createServer(common.mustNotCall()).listen({ fd: 42 })
+=======
+
+let invalidFd = 2;
+
+// Get first known bad file descriptor.
+try {
+  while (fs.fstatSync(++invalidFd));
+} catch (e) {
+  // do nothing; we now have an invalid fd
+}
+
+net.createServer(common.mustNotCall()).listen({ fd: invalidFd })
+>>>>>>> 1d8789188f... test: improve flaky test-listen-fd-ebadf.js
   .on('error', common.mustCall(onError));
 
 function onError(ex) {

gibfahn pushed a commit that referenced this pull request Jan 24, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Trott
Copy link
Member Author

Trott commented Jan 25, 2018

@gibfahn Merge conflict is caused by #14162 which added whitespace but hasn't yet been backported. Backport was requested in October so I guess it's not coming. That's unfortunate, because all those whitespace changes are likely to cause further merge conflicts. Oh well. At least it's easy to resolve: Remove the line from HEAD and keep all the lines from this PR. Done!

Trott added a commit to Trott/io.js that referenced this pull request Jan 25, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: nodejs#17797
Fixes: nodejs#17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Trott
Copy link
Member Author

Trott commented Jan 25, 2018

@gibfahn Backport in #18362.

MylesBorins pushed a commit that referenced this pull request Feb 5, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 7, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
Find an invalid file descriptor rather than assuming 42 will be invalid.

PR-URL: #17797
Fixes: #17762
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Trott Trott deleted the fix-flaky-ebadf branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky parallel/test-listen-fd-ebadf
6 participants