fix: use close event on response instead of socket

[josephharrington]

In #1880, we switched from using the close event on req to close on
req.socket. This addressed the intended issue but can trigger frequent
warnings when keep-alive is used due to a listener being added for each
request on the same socket.

By using the close event on res instead, we address both the issue of
event ordering in Node.js >= 16 that the original change was targeting
and the event emitter warning leak.

Fixes #1883.

[wesleytodd]

Am I missing something here? IIUC a client stopping in the middle of a POST (as the comment mentions), it could be that the res isn't closed, thus this might re-introduce the leak?

EDIT: I wonder if keeping both the socket and res close events would work?

[josephharrington]

Am I missing something here? IIUC a client stopping in the middle of a POST (as the comment mentions), it could be that the res isn't closed, thus this might re-introduce the leak?

I believe res is closed whenever req.socket is closed. The reason it's preferable to req's close event is that it is not affected by the autodestroy behavior change in Node.js 16.

I validated that the res close event reliably fires by manual testing of malformed and hanging requests, but if there's a specific flow you have in mind that would leave res open I am happy to try it (and add a test for it).

EDIT: I wonder if keeping both the socket and res close events would work?

Then we'd still have the event emitter leak warning, yes?

[mmarchini]

this should be fine and unless someone can come up with a scenario where this would leak memory/resources we should merge it (since it's better than the current state of things)

[svedbg]

Please release it in npm too.