-
Notifications
You must be signed in to change notification settings - Fork 565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Improve spec compliance by improve handling of responses without content-length or transfer-encoding #1540
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1540 +/- ##
==========================================
- Coverage 94.90% 94.89% -0.02%
==========================================
Files 50 50
Lines 4633 4638 +5
==========================================
+ Hits 4397 4401 +4
- Misses 236 237 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test. Also I need to think about this. Feels like there might be some edge cases here.Especially with pipelining.
It does have a test: ac2fbb9#diff-ecc2f3b14c967f02fd074ac2d0e0b876e683c84f2755d251bbaa74a813a82130 It just doesn't include any test where transfer-encoding is set.. which doesn't make a ton of sense to write an entire test for, but easy enough to do. |
Oh and the Ahh.. I think I see where your concern is around the edge cases and pipelining.. I would need to check, but it could be indeed possible that when you are pipelining, it would error out the next request. I am going to investigate. |
I think we should just error all the pipelined requests. Given they are GETs, they will be picked up whenever another socket is connected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test with more than one client.request()
executing in parallel with pipelining enabled and disabled? So we verify this works ok in all situations.
Hi everyone, any news about merging this? It will be very usefull for me to have it, thanks for the great work. |
This still needs one more test: #1540 (review) @evanderkoogh are you still interesting in completing this PR? |
@@ -615,6 +616,8 @@ class Parser { | |||
this.keepAlive += buf.toString() | |||
} else if (key.length === 14 && key.toString().toLowerCase() === 'content-length') { | |||
this.contentLength += buf.toString() | |||
} else if (key.length === 16 && key.toString().toLowerCase() === 'transfer-encoding') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by the line 617 (since I'm not very familiar with Undici's code), shouldn't this be 17
? transfer-encoding
has 3 more characters than the previous header, and .length
does indeed evaluate to 17
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a test is missing for this?
} | ||
|
||
parser.destroy() | ||
parser = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't properly null the parser. Please don't use a local var.
Would be great to get this merged. Seems like @evanderkoogh has done most of the heavy lifting. |
Would you mind sending a PR finishing this? |
This change somehow causes an lethal crash with SvelteKit sometimes. The root cause might be this one: nodejs/undici#1540
😢 So is this not getting fix for now? |
Any updates?
async fetchVideo(url, filepath) {
const filename = `${filepath}.mp4`;
return fetch(url)
// .then((res) => res.arrayBuffer())
.then((res) => {
console.log(`loading ... ⏳`);
return res.arrayBuffer();
})
.then((buffer) => {
console.log(`finished ✅`)
let arrayBuffer = Buffer.from(buffer)
fs.writeFile(filename, arrayBuffer, (err) => {
if (err) {
console.error(err);
console.error(`❌ video url`, url);
} else {
console.log("video downloaded successfully");
}
});
})
.catch((err) => console.error(`❌❌ video url`, url, err));
/*
❌❌ video url https://edu.xgqfrms.xyz/dc9bb2-1733219a4a8.mp4
TypeError: terminated
at Fetch.onAborted (node:internal/deps/undici/undici:11000:53)
at Fetch.emit (node:events:513:28)
at Fetch.terminate (node:internal/deps/undici/undici:10272:14)
at Object.onError (node:internal/deps/undici/undici:11095:36)
at Request.onError (node:internal/deps/undici/undici:6477:31)
at errorRequest (node:internal/deps/undici/undici:8440:17)
at TLSSocket.onSocketClose (node:internal/deps/undici/undici:7895:9)
at TLSSocket.emit (node:events:525:35)
at node:net:313:12
at TCP.done (node:_tls_wrap:587:7) {
[cause]: BodyTimeoutError: Body Timeout Error
at Timeout.onParserTimeout [as _onTimeout] (node:internal/deps/undici/undici:7839:32)
at listOnTimeout (node:internal/timers:566:11)
at process.processTimers (node:internal/timers:507:7) {
code: 'UND_ERR_BODY_TIMEOUT'
}
}
*/
}
|
PR has been closed for a while, but this seems to be still something to be considered to add. If interested, feel free to open a new PR with a proposal and we can outline something 🙂 |
The problem is that when I made that change, I fixed the actual issue, but then realised I introduced a bunch of edge-cases where there were still problems and I didn't have any bandwidth to figure out how to solve for all of those. |
I would have assumed a non-compliance with RFC 7230 3.3.3 should gain a bit more attention from the Node.js team than simply 'feel free to open a new PR'... Clearly this might be not a trivial problem to solve, which author of this PR already expressed in the above comment, hence any form of support from the team would be (very!) appreciated. |
The team has limited bandwidth for addressing all the issues opened at the time, hence we asked the community if they are willing to contribute to fixing the problem meanwhile we provide all the support and guidance through reviews. Since the PR has been closed for a while, it might be better to open a new one where we can continue the efforts if someone volunteers 🙂 |
This fixes #1414 #1412 #1490
From RFC 7230 3.3.3
So now if we get a SocketClosed, we check if we have seen a
content-length
ortransfer-encoding
header and if not, we complete the message parsing.