Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott 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

@TrottTrott 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
MemberAuthor

Ping @gibfahn

@Trott
Copy link
MemberAuthor

@gibfahn
Copy link
Member

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

@gibfahn
Copy link
Member

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-ebadfCommand: 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
MemberAuthor

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

@Trott
Copy link
MemberAuthor

Landed in 1d87891

@TrottTrott 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#17797Fixes: nodejs#17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorinsMylesBorins 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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@gibfahn
Copy link
Member

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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott
Copy link
MemberAuthor

@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#17797Fixes: nodejs#17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott
Copy link
MemberAuthor

@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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorinsMylesBorins 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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
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: #17797Fixes: #17762 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@TrottTrott deleted the fix-flaky-ebadf branch January 13, 2022 22:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

netIssues and PRs related to the net subsystem.testIssues 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

@Trott@gibfahn@jasnell@lpinca@XadillaX@BridgeAR