fix: Prevent unsubscribe from leaking listeners (#1295)

[felipec]

Since 8eeeec1 it seems listeners have been leaking on keep-alive sockets.

Apparently abort() has been deprecated since v17, using close() the onSocketClose listener is properly removed, and by creating a new onData function I'm able to remove the 'data' listener too.

---

• I updated ./docs/CHANGELOG.md with a link to this PR or Issue
• I updated ./docs/v3-UPGRADE-GUIDE
• I updated readme
• I added unit test(s)

---

• fix MaxListenersExceededWarning: Possible EventEmitter memory leak detected. #1295

[jimmywarting]

ping @LinusU to take a look

[AlttiRi]

Removing of

		request.on('abort', () => {
			socket.removeListener('close', onSocketClose);
		});

is OK, since I don't see any request.abort call. So, this thing is just useless.
More over request.abort was deprecated since Node.js v14.1.0/v13.14.0.

It was added in 51861e9 on Aug 12, 2021 by @tekwiz

---

Upd:
No, 'abort' listener is not useless, it's called when I abort fetch with AbortController. Here is it:

node-fetch/src/index.js

Line 101 in 004b3ac

request_.abort();

But anyway it worth to use 'close' event instead.

Also maybe it makes sense to use request.destroy instead of request.abort as the doc recommends?

[AlttiRi]

The other change is also the correct one. Removing the 'data' listener on the request closing.

The current code works obviously not-properly in case when the same socket is used for multiple requests. It will add additional listeners on each request.

The current implementation only suited for non keep-alive Agent — one request, one socket.

[AlttiRi]

BTW, the other way to prevent the existent of multiple listeners on the same net.Socket is storing a socket in WeakSet. Before adding the listener just check, is the socket already in the set, or no.

/** @type {WeakSet<net.Socket>} */
const socketsWithOnDataListener = new WeakSet();

// ...

if (!socketsWithOnDataListener.has(socket)) {
    socket.prependListener("data", onData);
}

Did not test, but should work.
In this case you only adds the listener once, instead of adding and removing it for the same socket for each request.

Upd:
Although, in this case it may not be acceptable, since the onData listener uses variables from the closure of each request.
So, it's better leave it as is in this pull request.

[github-actions]

🎉 This PR is included in version 3.2.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[rattrayalex]

Could this fix be backported to the 2.x line? Would a PR doing that be accepted/considered?

Context: #1295 (comment) user reported this in 2.6.0, and openai/openai-node#349 also reported this with node-fetch@^2.6.7.

(Further context, I help maintain the openai npm library, which will offer CJS support for as long as Node does).

EDIT: per this comment from Jimmy which says that bugfixes will be accepted to the 2.x branch, I'll see if we can put up a PR for that soon.