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: fix flaky test-gc-net-timeout #23139

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 28, 2018

There's a global.gc() invoked in an interval, and a second one in a
req.setTimeout() callback. Remove the one in the callback. I'm not sure
how competing global.gc() calls might result in a deadlock, but it seems
plausible and empirical testing confirms that it makes the test
reliable.

Fixes: #23067

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

There's a global.gc() invoked in an interval, and a second one in a
req.setTimeout() callback. Remove the one in the callback. I'm not sure
how competing global.gc() calls might result in a deadlock, but it seems
plausible and empirical testing confirms that it makes the test
reliable.

Fixes: nodejs#23067
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 28, 2018
@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 28, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this allow for moving back to parallel?

@BridgeAR
Copy link
Member

@nodejs/v8 could there be an issue with the gc feature?

@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

Since the issue this addresses is coming up a lot in CI, I'd like to fast-track this. 👍 here if you approve fast-tracking. Thanks.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 28, 2018
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 28, 2018
There's a global.gc() invoked in an interval, and a second one in a
req.setTimeout() callback. Remove the one in the callback. I'm not sure
how competing global.gc() calls might result in a deadlock, but it seems
plausible and empirical testing confirms that it makes the test
reliable.

Fixes: nodejs#23067

PR-URL: nodejs#23139
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

Landed in ea232bd

@Trott Trott closed this Sep 28, 2018
@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

Would this allow for moving back to parallel?

I'm reluctant because local testing with a big -j and --repeat still times out a lot. We could try it and move it back if it doesn't work out, but if we're going to do that sort of thing, I think I'd prefer to do it after Code & Learn. I want CI as reliable as possible for that.

targos pushed a commit that referenced this pull request Sep 29, 2018
There's a global.gc() invoked in an interval, and a second one in a
req.setTimeout() callback. Remove the one in the callback. I'm not sure
how competing global.gc() calls might result in a deadlock, but it seems
plausible and empirical testing confirms that it makes the test
reliable.

Fixes: #23067

PR-URL: #23139
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
There's a global.gc() invoked in an interval, and a second one in a
req.setTimeout() callback. Remove the one in the callback. I'm not sure
how competing global.gc() calls might result in a deadlock, but it seems
plausible and empirical testing confirms that it makes the test
reliable.

Fixes: #23067

PR-URL: #23139
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-gc-net-timeout
8 participants