-
Notifications
You must be signed in to change notification settings - Fork 525
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 parsing content-range header #2779
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2779 +/- ##
==========================================
- Coverage 85.54% 85.34% -0.21%
==========================================
Files 76 85 +9
Lines 6858 7602 +744
==========================================
+ Hits 5867 6488 +621
- Misses 991 1114 +123 ☔ View full report in Codecov by Sentry. |
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.
I don't think the retry handler changes are an improvement.
start: +m[1], | ||
end: (m[2] && +m[2]) ?? null, | ||
size: (m[3] && +m[3]) ?? 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.
Can you make this less compact and more readable? When how does this return null?
I will first implement some unit tests, as some of the expectations to the code seem to be wrong. |
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.
what about using the range header parsing from fetch?
sorry, didn't get a notification for your comment in the issue? |
Hmm, yeah looks right. Probably a magnitude slower. But I have stomacheache about the unsatisfied range case. |
* @see https://www.rfc-editor.org/rfc/rfc9110#field.content-range | ||
*/ | ||
function parseContentRangeHeader (range) { | ||
if (range == null || range === '') { |
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.
a header value can't be empty and I don't think range can be null here, only undefined
@@ -224,7 +230,7 @@ class RetryHandler { | |||
const { start, size, end = size } = contentRange |
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.
@KhafraDev
Here you see start, size and then end = size, which makes only sense in the unsatisfied case. But original parseRangeHeader can NOT return unsatisfied cases (e.g. bytes */2222
). So this makes no sense.
originally we only have start and end.
Sadly I don't see much room for improving the current Regarding |
If we abort on 416 i can implement it properly. Also it results in a faster parsing. |
Sure, that SGTM 👍 |
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.
lgtm
LOL, I dont want that this gets merged accidently. |
Regarding unsatisfied ranges:
The line
const { start, size, end = size } = contentRange
was only making sense, when the size was set but end was not set. This was not possible in the previous implementation. So I guess, that line was expected to handle unsatisfied ranges. So for cases like "bytes */1000". But I am unsure as there is no test regarding this case. Also we check if the statusCode is 206, while unsatisfied ranges are sent with status code 416.So i think we have to discuss about what we have to do with unsatisfied ranges.
benchmarks
before:
(last case is invalid in main and returns null, thats why it is in picoseconds...)
after: