test: fix child-process-fork-regr-gh-2847#5121
Uh oh!
There was an error while loading. Please reload this page.
Conversation
santigimeno commented Feb 6, 2016
mscdex added child_process 
Trott commented Feb 6, 2016
This one has been failing a lot on And here's a stress test using current master: (CI is closed off from public view for another couple of days until the security release of node is out, but I'll summarize the results here.) |
Trott commented Feb 6, 2016
A change that just landed on master today was to mark this test as flaky in |
Trott commented Feb 6, 2016
I stopped the master branch stress test because the test failed 5 times out of 9 runs. So there's the unsurprising confirmation that the test is highly flaky in current master. With this fix in place, the stress test has run 250 times without a single failure. So there's confirmation that this fix improves the situation. |
Trott commented Feb 6, 2016
@santigimeno Once you push the change to the |
The test would sometimes time out on some platforms. Take the initial version of the test and instead of sending 100 handles, just send 2. It still fails when run with the code before the change was introduced and passes afterwards. Remove test from parallel.status.
santigimenoforce-pushed the child-process-fork-regr-gh-2847 branch from cf59b37 to 65de3e4Comparesantigimeno commented Feb 6, 2016
@Trott PR updated. Thanks! |
Trott commented Feb 6, 2016
Trott commented Feb 7, 2016
mscdex commented Feb 7, 2016
Has this been stress tested on Windows? Also, I know this commit didn't change this part, but I'd be a bit concerned about the low timeout value (especially on pi1). |
Trott commented Feb 7, 2016
Stress tests on: |
| console.log(err); | ||
| sendcount++; | ||
| }); | ||
| // Send 2 handles to make `process.disconnect()` wait |
There was a problem hiding this comment.
I don't understand this comment. Where does the test call process.disconnect()?
There was a problem hiding this comment.
@bnoordhuis The original version of that comment landed in 36b969f and was authored by @indutny. I think it might be referring to target.disconnect() being called in lib/internal/child_process.js in channel.onread() but I'm not actually sure.
There was a problem hiding this comment.
Yes, what @Trott says. It was in the original version of the test. I can try to find out the reason
mscdex commented Feb 7, 2016
It looks like the pi1 stress test ran 0 times? |
Trott commented Feb 7, 2016
@mscdex That's what happens when I tell the test runner to run the test |
mhdawson commented Feb 9, 2016
I'm ok with the change provided it still catches the original problem and it passes on AIX. CI run on AIX here: https://ci.nodejs.org/job/node-test-commit-aix/37/ (unfortunately is does take a while to run) |
mscdex commented Feb 9, 2016
LGTM |
Trott commented Feb 9, 2016
I confirmed that the test still fails (as expected) in Node.js v3.3.1 (with ES6-isms removed from LGTM but awaiting AIX results... |
mhdawson commented Feb 9, 2016
lgtm tests still passs on AIX. |
The test would sometimes time out on some platforms. Take the initial version of the test and instead of sending 100 handles, just send 2. It still fails when run with the code before the change was introduced and passes afterwards. Remove test from parallel.status. PR-URL: nodejs#5121 Reviewed-By: Brian White <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Fixes: nodejs#3635
Trott commented Feb 10, 2016
Landed in 4e4b260 Feels good to get that one off the flaky list...Thanks, @santigimeno ! |
The test would sometimes time out on some platforms. Take the initial version of the test and instead of sending 100 handles, just send 2. It still fails when run with the code before the change was introduced and passes afterwards. Remove test from parallel.status. PR-URL: #5121 Reviewed-By: Brian White <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Fixes: #3635
The test would sometimes time out on some platforms. Take the initial version of the test and instead of sending 100 handles, just send 2. It still fails when run with the code before the change was introduced and passes afterwards. Remove test from parallel.status. PR-URL: #5121 Reviewed-By: Brian White <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Fixes: #3635
The test would sometimes time out on some platforms. Take the initial version of the test and instead of sending 100 handles, just send 2. It still fails when run with the code before the change was introduced and passes afterwards. Remove test from parallel.status. PR-URL: #5121 Reviewed-By: Brian White <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Fixes: #3635
The test would sometimes time out on some platforms. Take the initial version of the test and instead of sending 100 handles, just send 2. It still fails when run with the code before the change was introduced and passes afterwards. Remove test from parallel.status. PR-URL: #5121 Reviewed-By: Brian White <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Fixes: #3635
According to the explanation in nodejs#3635#issuecomment-157714683 And as a continuation to nodejs#5422 we also ignore EMFILE "No more file descriptors are available,so no more files can be opened" PR-URL: nodejs#12698Fixes: nodejs#10286 Refs: nodejs#3635 (comment) Refs: nodejs#5178 Refs: nodejs#5179 Refs: nodejs#4005 Refs: nodejs#5121 Refs: nodejs#5422 Refs: nodejs#12621 (comment) Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
According to the explanation in #3635#issuecomment-157714683 And as a continuation to #5422 we also ignore EMFILE "No more file descriptors are available,so no more files can be opened" PR-URL: #12698Fixes: #10286 Refs: #3635 (comment) Refs: #5178 Refs: #5179 Refs: #4005 Refs: #5121 Refs: #5422 Refs: #12621 (comment) Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
7 participants
The test would sometimes time out on some platforms. Take the initial
version of the test and instead of sending 100 handles, just send 2.
It still fails when run with the code before the change was introduced
and passes afterwards.