test: fix unreliable test-gc-http-client

[Trott]

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]

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1049/pipeline

[Trott]

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

[Trott]

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

[targos]

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

[targos]

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

[targos]

Sorry, this test is not fixed for me

[addaleax]

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

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.

might be an improvement, but doesn't fix flakyness

[Trott]

Stress test this PR: https://ci.nodejs.org/job/node-stress-single-test/2031/

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

[Trott]

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

[Trott]

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]

ARM rebuild: https://ci.nodejs.org/job/node-test-commit-arm/18818/ ✔️

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

[targos]

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

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

[refack]

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

[Trott]

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]

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

[Trott]

Landed in 87ea0ca