nodejs
fix: Improve spec compliance by improve handling of responses without content-length or transfer-encoding
This fixes #1414 #1412 #1490
From RFC 7230 3.3.3
Otherwise, this is a response message without a declared message
body length, so the message body length is determined by the
number of octets received prior to the server closing the
connection.
So now if we get a SocketClosed, we check if we have seen a content-length or transfer-encoding header and if not, we complete the message parsing.
Codecov Report
Merging #1540 (ac2fbb9) into main (c485884) will decrease coverage by
0.01%. The diff coverage is87.50%.
@@ 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
| Impacted Files | Coverage Δ | |
|---|---|---|
| lib/client.js | 97.64% <87.50%> (-0.12%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update c485884...ac2fbb9. Read the comment docs.
It does have a test: https://github.com/nodejs/undici/pull/1540/commits/ac2fbb96e612b6cbb999a956fbe03a66673d937f#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 Node CI / Test / test (18, ubuntu-latest, experimental) (pull_request) Failing after 2m failing test is a timeout in some unrelated code.
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.
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: https://github.com/nodejs/undici/pull/1540#pullrequestreview-1038547402
@evanderkoogh are you still interesting in completing this PR?
Would be great to get this merged. Seems like @evanderkoogh has done most of the heavy lifting.
