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 unreliable test-gc-http-client #23145

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 28, 2018

Calling global.gc() in multiple places leads to unreliability. Call it
in the interval only.

Fixes: #22336

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

@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

Would like to fast-track this. 👍 if you approve.

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

Trott commented Sep 28, 2018

@targos
Copy link
Member

targos commented Sep 28, 2018

Does it really fix #23066? It's not modifying the test in question.

@targos
Copy link
Member

targos commented Sep 28, 2018

Please remove the "Fixes:" link from the commit message.

targos
targos previously requested changes Sep 28, 2018
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Sorry, this test is not fixed for me

@addaleax
Copy link
Member

@targos Just to be clear, your red X is about the Fixes: tag, not the diff itself, right? Because I think this is definitely an improvement.

@targos
Copy link
Member

targos commented Sep 28, 2018

After applying this change, I ran: ./tools/test.py --repeat 100 test/sequential/test-gc-http-client.js

The test always fails before the 30th execution.

@targos targos dismissed their stale review September 28, 2018 14:28

might be an improvement, but doesn't fix flakyness

Calling `global.gc()` in multiple places leads to unreliability. Call it
in the interval only.

Fixes: nodejs#22336
@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

Given CI stress test results (8 failures in 1000 runs on master vs. 0 failures in 1000 runs with this PR), I'm inclined to believe that "Fixes:" is correct here, except for @targos reporting that it is still happening locally...

@targos Is there any chance at all that you were testing against master and not this PR or something like that? Can you share a gist of you at least some of your output and also what platform/OS you're on?

@Trott
Copy link
Member Author

Trott commented Sep 28, 2018

@targos
Copy link
Member

targos commented Sep 29, 2018

@Trott

@targos Is there any chance at all that you were testing against master and not this PR or something like that? Can you share a gist of you at least some of your output and also what platform/OS you're on?

I'm certain that I am testing against this PR applied on top of master.
Gist (truncated after 10 runs): https://gist.github.com/targos/2b9fd900456e64a6fd2f7245d9e18125

My platform is Linux (Fedora 28)

If this fixes the issue on CI, I'm not blocking it.

@Trott
Copy link
Member Author

Trott commented Sep 29, 2018

@targos Thanks! Interesting! I think your gist is different than what we're seeing in CI, I think, because Done: always gets up to 300/300 and it's Collected: that stalls. But for you, it looks like Done: stalled... Any chance there's some firewall throttling going on or something like that?

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

refack commented Oct 1, 2018

Resume CI (just for good measure): https://ci.nodejs.org/job/node-test-commit/21949/

@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

Windows timed out so will definitely remove the Fixes, just like @targos suggested. (Stress tests showing failures on master and clean runs with this PR were run on AIX.)

ARM rebuild: https://ci.nodejs.org/job/node-test-commit-arm/18888/

Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/21186/

@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

@Trott
Copy link
Member Author

Trott commented Oct 2, 2018

Landed in 87ea0ca

@Trott Trott closed this Oct 2, 2018
Trott added a commit to Trott/io.js that referenced this pull request Oct 2, 2018
Calling `global.gc()` in multiple places leads to unreliability. Call it
in the interval only.

PR-URL: nodejs#23145
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
Calling `global.gc()` in multiple places leads to unreliability. Call it
in the interval only.

PR-URL: #23145
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the fix-23066 branch January 13, 2022 22:50
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 parallel/test-gc-http-client
9 participants