Revert "http: use Keep-Alive by default in global agents"

[F3n67u]

This reverts commit 4267b92.

Fix#43623

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/http
• @nodejs/net

[ShogunPanda]

Why do you want to revert that?

[F3n67u]

Why do you want to revert that?

@ShogunPanda try to fix #43623 (comment)

[ShogunPanda]

Don't revert that, it is needed.
I'll briefly send a PR to fix the failing test.

[F3n67u]

Don't revert that, it is needed.
I'll briefly send a PR to fix the failing test.

Thanks.

[F3n67u]

#43522 also causes test-gc-http-client-timeout to fail, so I reopen this revert pr. Refs: #43638 (comment)

[ShogunPanda]

See: #43641 (comment)

[panva]

I don't understand the urgency not to revert. It is semver major, not to be part of a release until october.

Revert it, reopen it with ci fixes, run it a bunch of times to be more confident no more flakyness was introduced, then land.

[nodejs-github-bot]

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

[ShogunPanda]

I explained in my PR.
IMHO the new behavior exposed some problems in the tests that needs to be addressed. Reverting would hide them again so I would suggest to keep it and face problems as they arise.

As you pointed out, that was a semver-major so we're not blocking any release line I guess.

[tniessen]

IMHO the new behavior exposed some problems in the tests that needs to be addressed.

@ShogunPanda Yes, but that should have been part of #43522. There were related test failures (in four out of five CI runs!), which appear to have been ignored, and the PR was merged despite these failures. Arguably, when a PR should not have been merged in the first place, reverting it is a good first step.

Reverting would hide them again so I would suggest to keep it and face problems as they arise.

In general, that is a good approach. In this case, however, the introduced CI failures are blocking others' work. PRs can't be merged because CIs keep failing. Even CI jobs that are usually reliable have been failing much more often.

For example, test-commit-linux worked pretty well (with the vast majority of builds succeeding) until three days ago:

Similar things can be said about linuxone etc.

CI failures mean that we might miss important errors (as appears to be the case in #43522) and they cost us time and resources.

As you pointed out, that was a semver-major so we're not blocking any release line I guess.

As @panva explained, the change isn't being released until node 19 anyway, so (most) users do not benefit from it being on the main branch. It's not blocking a release line, but it is blocking development on the main branch!

[ShogunPanda]

Fine to me then. I can probably propose a combined version of #43522 and #43641.
What would be an indicator of green light in that case? CI passing at first run or what?

[tniessen]

What would be an indicator of green light in that case? CI passing at first run or what?

No, that's not a requirement (and also does not always mean that no flakes are introduced). The important bit is looking at the actual errors when CI fails. Unfortunately, the degree of flakiness of our CI makes it tempting to just rerun/resume CI until it passes. test-stream-finished failed at least four times throughout multiple CI runs in #43522 and that test was not a known flake, so the failure should have been investigated.

[ShogunPanda]

I see. Anyway, now that #43641 has been now merged, shall we wait few more days to see if the situation has improved?

[F3n67u]

Close this pr to in favor of #43949