fix: disable chunked encoding fix with keep-alive requests

[tekwiz]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• New feature
• Other, please explain:

What changes did you make? (provide an overview)

This change disables fixResponseChunkedTransferBadEnding when the request is a keep-alive request.

Which issue (if any) does this pull request address?

• fix MaxListenersExceededWarning: Possible EventEmitter memory leak detected. #1295
• fix Memory leak in v3.1.0 #1399

Is there anything you'd like reviewers to know?

[jimmywarting]

I have been thinking if we maybe could use finalizers to clean up stuff...
also for when the body is never consumed like in var just_want_header = await fetch(url).then(res => res.headers)

edit: But now i see that it don't exist in node 12...

[tekwiz]

@jimmywarting @xxczaki @Richienb Can I get one of you to review? It was confirmed in the thread for #1295 that this fixes that issue, and I just confirmed that it fixes #1446 as well.

[jimmywarting]

i guess not many won't even pass in the shouldKeepAlive option in the request, it's just another node-fetch specific option, I'm always a bit hesitated adding node-fetch specific option into node-fetch that isn't part of the spec. We should try to automatically solve this kind of issues if we can, So that developers don't have to worry about such edge cases.

Nowhere in the readme is there any mention of the option shouldKeepAlive option.
so the readme needs to be updated if we want to expose this shouldKeepAlive option.

basically the fixResponseChunkedTransferBadEnding will likely never be used cuz nobody is using the new shouldKeepAlive option today... will this change improve things rather then bringing back an old issue that the fixResponseChunkedTransferBadEnding function is doing?

For the v4 milestone we will likely not be using pipeline anymore as we will transition to whatwg streams and instead uses pipeThrough and pipeTo... do you think it will have any impact when we ditch the pipeline?

v4 will also likely drop support for NodeJS below v14.17

[jimmywarting]

ping @tekwiz

also what do you think about this PR #1474?

[AlttiRi]

@jimmywarting

fixResponseChunkedTransferBadEnding has the questionable code (see my comments), #1474 should be accepted in any case.

---

About this #1325 pull request:

In case of keep-alive Agent the fixResponseChunkedTransferBadEnding is not needed? Otherwise this pull request looks like just the problem ignoring by passing some property (with breaking the code (if fixResponseChunkedTransferBadEnding is really the required function, although I'm not sure)).

[AlttiRi]

request.shouldKeepAlive is a native property of the request object.
Although, there is no note about it in the doc, but it is.
It looks to be OK approach to detect is request uses keep-alive Agent, or no.

Alternative property: request.agent.keepAlive.

[AlttiRi]

Well, it seems for me that fixResponseChunkedTransferBadEnding is not suited to work with keep-alive Agent at all.

Since it relies on 'close' event of Socket, however, there will no 'close' event with keep-alive Agent after the chunked encoding request end until the application is closed, for example.

Anyway this thing is not blocking for #1474.

---

BTW, based on comments in this issue #1219 it makes sense to use fixResponseChunkedTransferBadEnding only if Node.js version is lower than 16.

if (process.version < 'v16') {
    fixResponseChunkedTransferBadEnding(request_, error => {
        response.body.destroy(error);
    });
}

Node.js 14 will be actual yet 12 months:
https://nodejs.org/en/about/releases/

[AlttiRi]

Should say, since fixResponseChunkedTransferBadEnding is not suited for keep-alive Agent at all this pull request makes sense.
Probably, even more than the other one. While #1474 fixes the memory leak, but one listener will exist all time and it will do a useless work.