http: detach socket from IncomingMessage on keep-alive

[ronag]

If the socket is not detached then a future call to res.destroy
(through e.g. pipeline) would unecessarily kill the socket while
its in the agent free list.

Checklist

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

[ronag]

@szmarczak I think this is the fix you were looking for.

[ronag]

I'm not sure whether setting socket to null here can cause more breakage... but it does make sense to set it to null. Also given the quote above IncomingMessage.destroy

// It's possible that the socket will be destroyed, and removed from
// any messages, before ever calling this.  In that case, just skip
// it, since something else is destroying this connection anyway.

[szmarczak]

Looks amazing, you're a lifesaver!

[addaleax]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2296/

[nodejs-github-bot]

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

[ronag]

CITGM looks good

[ronag]

Landed in c1b2f6a

[szmarczak]

Since this isn't a breaking change, will this be backported?

[ronag]

@szmarczak it's labeled as semver-major, sorry :(

[Jimbly]

This looks like it's no longer testing what it used to be testing (just working around the fact that response.socket is cleared now by attaching the event to an unrelated socket?). Proper fix would be to leave the event on response.socket and just attach it before the socket is cleared (up 3 lines, right after response.pipe)?

I'm not actually familiar with this code, just in here since this test started breaking on Node v14 over in http-parser-js, which has a fork of some of these tests, but I noticed the old code also had a big comment about how important the timing/ordering of attaching that event listener was.