Skip to content
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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions benchmarks/parseContentRangeHeader.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { bench, group, run } from 'mitata'
import { parseContentRangeHeader } from '../lib/core/util.js'

group('parseContentRangeHeader', () => {
bench('parseContentRangeHeader undefined', () => {
parseContentRangeHeader()
})
bench('parseContentRangeHeader empty', () => {
parseContentRangeHeader('')
})
bench('parseContentRangeHeader bytes 0-400/400', () => {
parseContentRangeHeader('bytes 0-400/400')
})
bench('parseContentRangeHeader bytes 0-400/*', () => {
parseContentRangeHeader('bytes 0-400/*')
})
})

await run()
50 changes: 36 additions & 14 deletions lib/core/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -487,19 +487,41 @@ function isValidHTTPToken (characters) {
return true
}

// Parsed accordingly to RFC 9110
// https://www.rfc-editor.org/rfc/rfc9110#field.content-range
function parseRangeHeader (range) {
if (range == null || range === '') return { start: 0, end: null, size: null }

const m = range ? range.match(/^bytes (\d+)-(\d+)\/(\d+)?$/) : null
return m
? {
start: parseInt(m[1]),
end: m[2] ? parseInt(m[2]) : null,
size: m[3] ? parseInt(m[3]) : null
}
: null
const emptyRange = Object.freeze({ start: 0, end: null, size: null })
const rangeHeaderRE = /^bytes (?:(?:(?:(\d+)-(\d+)))\/(?:\*|(\d+))$|\*\/(\d+))$/

/**
* @typedef {Object} SatisfiedContentRangeHeader
* @property {number} start
* @property {number} end
* @property {number | null} size
*/

/**
* @typedef {Object} UnsatisfiedContentRangeHeader
* @property {0} start
* @property {null} end
* @property {number} size
*/

/**
* Parsed accordingly to RFC 9110
*
* @param {null|string} range
* @returns {null|SatisfiedContentRangeHeader|UnsatisfiedContentRangeHeader}
* @see https://www.rfc-editor.org/rfc/rfc9110#field.content-range
*/
function parseContentRangeHeader (range) {
if (range == null || range === '') {
Copy link
Member

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

return emptyRange
}

const m = range.match(rangeHeaderRE)
return m && ((m[4] && { start: 0, end: null, size: +m[4] }) || {
start: +m[1],
end: (m[2] && +m[2]) ?? null,
size: (m[3] && +m[3]) ?? null
})
Copy link
Member

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?

}

const kEnumerableProperty = Object.create(null)
Expand Down Expand Up @@ -538,7 +560,7 @@ module.exports = {
addAbortListener,
isValidHTTPToken,
isTokenCharCode,
parseRangeHeader,
parseContentRangeHeader,
nodeMajor,
nodeMinor,
nodeHasAutoSelectFamily: nodeMajor > 18 || (nodeMajor === 18 && nodeMinor >= 13),
Expand Down
27 changes: 16 additions & 11 deletions lib/handler/RetryHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const assert = require('node:assert')

const { kRetryHandlerDefaultRetry } = require('../core/symbols')
const { RequestRetryError } = require('../core/errors')
const { isDisturbed, parseHeaders, parseRangeHeader } = require('../core/util')
const { isDisturbed, parseHeaders, parseContentRangeHeader } = require('../core/util')

function calculateRetryAfterHeader (retryAfter) {
const current = Date.now()
Expand Down Expand Up @@ -58,7 +58,13 @@ class RetryHandler {
}

this.retryCount = 0
/**
* @type {number}
*/
this.start = 0
/**
* @type {number | null}
*/
this.end = null
this.etag = null
this.resume = null
Expand Down Expand Up @@ -198,9 +204,9 @@ class RetryHandler {
return true
}

const contentRange = parseRangeHeader(headers['content-range'])
const contentRange = parseContentRangeHeader(headers['content-range'])
// If no content range
if (!contentRange) {
if (contentRange === null) {
this.abort(
new RequestRetryError('Content-Range mismatch', statusCode, {
headers,
Expand All @@ -224,7 +230,7 @@ class RetryHandler {
const { start, size, end = size } = contentRange
Copy link
Contributor Author

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.


assert(this.start === start, 'content-range mismatch')
assert(this.end == null || this.end === end, 'content-range mismatch')
assert(this.end === null || this.end === end, 'content-range mismatch')

this.resume = resume
return true
Expand All @@ -233,9 +239,9 @@ class RetryHandler {
if (this.end == null) {
if (statusCode === 206) {
// First time we receive 206
const range = parseRangeHeader(headers['content-range'])
const contentRange = parseContentRangeHeader(headers['content-range'])

if (range == null) {
if (contentRange === null) {
return this.handler.onHeaders(
statusCode,
rawHeaders,
Expand All @@ -244,15 +250,14 @@ class RetryHandler {
)
}

const { start, size, end = size } = range
const { start, size, end = size } = contentRange

assert(
start != null && Number.isFinite(start) && this.start !== start,
this.start !== start,
'content-range mismatch'
)
assert(Number.isFinite(start))
assert(
end != null && Number.isFinite(end) && this.end !== end,
end !== null && this.end !== end,
'invalid content-length'
)

Expand All @@ -268,7 +273,7 @@ class RetryHandler {

assert(Number.isFinite(this.start))
assert(
this.end == null || Number.isFinite(this.end),
this.end === null || Number.isFinite(this.end),
'invalid content-length'
)

Expand Down
58 changes: 58 additions & 0 deletions test/utils/parse-content-range-header.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
'use strict'

const { strictEqual, deepStrictEqual } = require('node:assert')
const { test, describe } = require('node:test')
const { parseContentRangeHeader } = require('../../lib/core/util')

describe('parseContentRangeHeader', () => {
test('empty string', () => {
deepStrictEqual(parseContentRangeHeader(''), { start: 0, end: null, size: null })
strictEqual(Object.isFrozen(parseContentRangeHeader('')), true)
})

test('undefined', () => {
deepStrictEqual(parseContentRangeHeader(), { start: 0, end: null, size: null })
strictEqual(Object.isFrozen(parseContentRangeHeader()), true)
})

test('null', () => {
deepStrictEqual(parseContentRangeHeader(), { start: 0, end: null, size: null })
strictEqual(Object.isFrozen(parseContentRangeHeader()), true)
})

test('invalid', () => {
deepStrictEqual(parseContentRangeHeader('invalid'), null)
})

test('bytes */*', () => {
deepStrictEqual(parseContentRangeHeader('bytes */*'), null)
})

test('bytes 0-2', () => {
deepStrictEqual(parseContentRangeHeader('bytes 0-2'), null)
})

test('bytes 0-2/', () => {
deepStrictEqual(parseContentRangeHeader('bytes 0-2/'), null)
})

test('bytes 0-400/400', () => {
deepStrictEqual(parseContentRangeHeader('bytes 0-400/400'), { start: 0, end: 400, size: 400 })
})

test('bytes 1-400/400', () => {
deepStrictEqual(parseContentRangeHeader('bytes 1-400/400'), { start: 1, end: 400, size: 400 })
})

test('bytes 1-400/*', () => {
deepStrictEqual(parseContentRangeHeader('bytes 1-400/*'), { start: 1, end: 400, size: null })
})

test('bytes 1-400/0', () => {
deepStrictEqual(parseContentRangeHeader('bytes 1-400/0'), { start: 1, end: 400, size: 0 })
})

test('bytes */400', () => {
deepStrictEqual(parseContentRangeHeader('bytes */400'), { start: 0, end: null, size: 400 })
})
})
Loading