test: fix flaky child-process-fork-regr-gh-2847 by santigimeno · Pull Request #5422 · nodejs/node

santigimeno · 2016-02-24T23:02:38Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

test

Description of change

The test is still failing sometimes because when trying to establish the
second connection, the server is already closed. Bring back the code
that handled this case and was removed in the last refactoring of the
test: 4e4b260.

mscdex · 2016-02-24T23:15:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1734/

The CI infrastructure isn't all back up yet, but some nodes are.

Trott · 2016-02-24T23:21:41Z

LGTM if CI is green.

I de-ES6-ized it and made sure it still failed on io.js v3 so it still catches the regression it's designed to catch.

mscdex · 2016-02-25T04:36:38Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/1742/

mscdex · 2016-02-25T06:15:10Z

Stress test: https://ci.nodejs.org/job/node-stress-single-test/525/

santigimeno · 2016-02-25T08:47:37Z

2 failures out of 9999. Things have improved but not completely :(

santigimeno · 2016-02-25T09:52:22Z

Updated the PR so it ignores errors when sending the 2nd handle to the worker

mscdex · 2016-02-25T10:13:34Z

Perhaps it would be better to at least check that it's a possibly expected error?

santigimeno · 2016-02-25T10:23:08Z

PR update to address @mscdex comment. Thanks

mscdex · 2016-02-25T10:27:04Z

New stress test: https://ci.nodejs.org/job/node-stress-single-test/526/

Trott · 2016-02-25T18:54:55Z

Nit: Might be good just to follow the format of the previous case of swallowing ECONNREFUSED in the file: https://github.com/santigimeno/node/blob/regre-gh-2847/test/parallel/test-child-process-fork-regr-gh-2847.js#L19-L23

Trott · 2016-02-25T18:56:28Z

If it passes CI and stress test, LGTM. (Left a nit, but like all nits, it can be ignored. Up to you.)

Trott · 2016-02-25T18:56:54Z

Oh, and I ran the current version (with ES6-isms replaced) against iojs v3 and it fails as expected, so that's good.

santigimeno · 2016-02-25T21:35:57Z

In last stress test, there was 8 failures because I was only ignoring ECONNREFUSED error. Should we make a new run with logs to see what other errors are happening, or just ignore every error as it was before the first commit I made here: 4e4b260?
/cc @mscdex @Trott

mscdex · 2016-02-25T21:43:37Z

@santigimeno Maybe try the logging first?

santigimeno · 2016-02-25T21:50:45Z

Ok, I have followed @Trott recommendations and throw if error is different from ECONNREFUSED. Could you run the stress test again? Thanks

mscdex · 2016-02-25T21:54:06Z

New stress test: https://ci.nodejs.org/job/node-stress-single-test/527/

mhdawson · 2016-02-26T21:22:57Z

Seen again https://ci.nodejs.org/job/node-test-commit-plinux/1514/

Trott · 2016-02-26T23:05:17Z

@santigimeno I think it will work if you swallow ECONNREFUSED on line 51 (like you are already doing) and swallow ECONNRESET on line 65 (instead of ECONNREFUSED).

santigimeno · 2016-02-27T07:31:56Z

@Trott yes, but there are other ECONNREFUSED that I'm not sure where they come from. From https://ci.nodejs.org/job/node-stress-single-test/527/:

not ok 1 test-child-process-fork-regr-gh-2847.js
# events.js:155
# throw er; // Unhandled 'error' event
# ^
# # Error: connect ECONNREFUSED 127.0.0.1:12346
# at Object.exports._errnoException (util.js:859:11)
# at exports._exceptionWithHostPort (util.js:882:20)
# at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1059:14)

I will push a new PR with more logs

santigimeno · 2016-02-27T07:45:50Z

Can somebody run the stress test again? Thanks

mscdex · 2016-02-27T20:28:53Z

New stress test: https://ci.nodejs.org/job/node-stress-single-test/546/

Trott · 2016-02-27T21:17:37Z

And just in case it's challenging to find the corresponding console log: https://ci.nodejs.org/job/node-stress-single-test/546/nodes=ppcbe-fedora20/console

santigimeno · 2016-02-28T07:22:30Z

I think (hope) it's now fixed. Can you run the stress test? Thanks!

mscdex · 2016-02-28T09:11:42Z

New stress test: https://ci.nodejs.org/job/node-stress-single-test/551/

Trott · 2016-02-28T21:41:48Z

That looks like it worked.

CI: https://ci.nodejs.org/job/node-test-commit/2371/

The test is still failing sometimes because when trying to establish the second connection, the server is already closed. Bring back the code that handled this case and was removed in the last refactoring of the test. Also ignore the errors that might happen when sending the second handle to the worker because it may already have exited.

santigimeno · 2016-02-29T08:49:52Z

Fixed linting error.

Trott · 2016-02-29T21:47:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1782/

mscdex · 2016-02-29T23:08:46Z

CI is green.

The test is still failing sometimes because when trying to establish the second connection, the server is already closed. Bring back the code that handled this case and was removed in the last refactoring of the test. Also ignore the errors that might happen when sending the second handle to the worker because it may already have exited. PR-URL: nodejs#5422 Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott · 2016-02-29T23:43:36Z

Landed in 98b721e

The test is still failing sometimes because when trying to establish the second connection, the server is already closed. Bring back the code that handled this case and was removed in the last refactoring of the test. Also ignore the errors that might happen when sending the second handle to the worker because it may already have exited. PR-URL: #5422 Reviewed-By: Rich Trott <rtrott@gmail.com>

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 <gibfahn@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <gibfahn@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

santigimeno mentioned this pull request Feb 24, 2016
test: child-process-fork-regr-gh-2847 is flaky still #5349
Closed

mscdex added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Feb 24, 2016

santigimeno force-pushed the regre-gh-2847 branch from fa836e7 to a83b311Compare February 25, 2016 09:50

mscdex reviewed Feb 25, 2016
View reviewed changes

santigimeno force-pushed the regre-gh-2847 branch 2 times, most recently from 2dfbfe9 to 4e8a0caCompare February 25, 2016 10:22

Trott reviewed Feb 25, 2016
View reviewed changes

santigimeno force-pushed the regre-gh-2847 branch from 4e8a0ca to e1a3059Compare February 25, 2016 21:48

santigimeno force-pushed the regre-gh-2847 branch from e1a3059 to 1ba5d2bCompare February 27, 2016 07:45

santigimeno force-pushed the regre-gh-2847 branch from 1ba5d2b to f192900Compare February 28, 2016 07:21

santigimeno force-pushed the regre-gh-2847 branch from f192900 to 1e36bc0Compare February 29, 2016 08:48

Trott closed this Feb 29, 2016

Fishrock123 mentioned this pull request Mar 1, 2016
Release proposal: v5.7.1 #5464
Merged
5 tasks

jasnell added the lts-watch-v4.x label Mar 2, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 17, 2016

refack mentioned this pull request Apr 27, 2017
test: ignore spurious 'EMFILE' #12698
Merged
3 tasks

Uh oh!

Uh oh!

Conversation

santigimeno commented Feb 24, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Feb 24, 2016

Uh oh!

Trott commented Feb 24, 2016

Uh oh!

mscdex commented Feb 25, 2016

Uh oh!

mscdex commented Feb 25, 2016

Uh oh!

santigimeno commented Feb 25, 2016

Uh oh!

santigimeno commented Feb 25, 2016

Uh oh!

mscdexFeb 25, 2016

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Feb 25, 2016

Uh oh!

mscdex commented Feb 25, 2016

Uh oh!

TrottFeb 25, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 25, 2016

Uh oh!

Trott commented Feb 25, 2016

Uh oh!

santigimeno commented Feb 25, 2016

Uh oh!

mscdex commented Feb 25, 2016

Uh oh!

santigimeno commented Feb 25, 2016

Uh oh!

mscdex commented Feb 25, 2016

Uh oh!

mhdawson commented Feb 26, 2016

Uh oh!

Trott commented Feb 26, 2016

Uh oh!

santigimeno commented Feb 27, 2016

Uh oh!

santigimeno commented Feb 27, 2016

Uh oh!

mscdex commented Feb 27, 2016

Uh oh!

Trott commented Feb 27, 2016

Uh oh!

santigimeno commented Feb 28, 2016

Uh oh!

mscdex commented Feb 28, 2016

Uh oh!

Trott commented Feb 28, 2016

Uh oh!

santigimeno commented Feb 29, 2016

Uh oh!

Trott commented Feb 29, 2016

Uh oh!

mscdex commented Feb 29, 2016

Uh oh!

Trott commented Feb 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants