fix: disable chunked encoding fix with keep-alive requests #1325
Conversation
|
I have been thinking if we maybe could use finalizers to clean up stuff... edit: But now i see that it don't exist in node 12... |
|
@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
changed the title
| fixResponseChunkedTransferBadEnding(request_, error => { | ||
| response.body.destroy(error); | ||
| }); | ||
| if (!request_.shouldKeepAlive) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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.
|
Well, it seems for me that Since it relies on Anyway this thing is not blocking for #1474. BTW, based on comments in this issue #1219 it makes sense to use if (process.version < 'v16') {
fixResponseChunkedTransferBadEnding(request_, error => {
response.body.destroy(error);
});
}Node.js 14 will be actual yet 12 months: |
|
Should say, since |
What is the purpose of this pull request?
What changes did you make? (provide an overview)
This change disables
fixResponseChunkedTransferBadEndingwhen the request is a keep-alive request.Which issue (if any) does this pull request address?
Is there anything you'd like reviewers to know?