test: fix flaky test-http-writable-true-after-close

[apapirovski]

Fixes: #16321

This will need full stress test CI, coming shortly. (But it did fix it on my macOS.)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test

[apapirovski]

CI: https://ci.nodejs.org/job/node-test-pull-request/12373/
Stress Test CI:
https://ci.nodejs.org/job/node-stress-single-test/1593/ (green)
https://ci.nodejs.org/job/node-stress-single-test/1594/ (green)

[mcollina]

I'm -1 for this change. This removes the scope of the test itself, we are better off removing it altogether.

I'm kind of concerned that this might hide some underlining bugs or memory leak. res should close/finish/error, why is this not happening?

[lpinca]

res should close/finish/error, why is this not happening?

Test description is misleading then as it says

// res.writable should not be set to false after it has finished sending

and this change actually tests that.

[mcollina]

// res.writable should not be set to false after it has finished sending

Means when the outer request res has transmitted data. This PR tests that when the inner request has ended, res is still writable.

[lpinca]

I think the assumption (@apapirovski please correct me if I'm wrong) is that on next tick after inner res has finished writing, outer res has also finished writing, but yes maybe this is a wrong assumption.

[apapirovski]

I think the assumption (@apapirovski please correct me if I'm wrong) is that on next tick after inner res has finished writing, outer res has also finished writing, but yes maybe this is a wrong assumption.

That's correct and the test does fail on older Node versions.

[apapirovski]

Also IMO if there's a bug hiding here then it's unrelated to the description of this test and should honestly have its own test. The fact that finished and close have to both be used, is indicative of this test being bit too dependent on timing.

[mcollina]

It’s not a timing issue, there is something more subtle going on.

[mcollina]

LGTM

[apapirovski]

@mcollina @lpinca PTAL, I've traced down the issue and adjusted the test. It appears that there are situations where the socket associated with res is closed before the callback for the get is called and as a result the event handlers are bound too late.

[apapirovski]

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1610/
CI: https://ci.nodejs.org/job/node-test-pull-request/12397/

[mcollina]

LGTM

[BridgeAR]

Landed in 9cef060