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.

Sign up for GitHub

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

Jump to bottom

feat: add maxChunkSize option #90

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

feat: add maxChunkSize option #90

wants to merge 12 commits into from
+76 −2

Conversation

ledyba
Copy link

@ledyba ledyba commented Nov 16, 2024 •
edited
Loading

Currently, this library tends to send entire file when 'acceptRanges': true,

In this PR, I added maxChunkSize to send.SendOptions to limit the max chunk size.

Test case: https://hexe.net/2022/09/17/17:04:55/
Souce code is here

I'm using firefox 132.0.

Request:

GET /entity/eb3467a204907295921f367ac8187d67 HTTP/2
Host: hexe.net
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:132.0) Gecko/20100101 Firefox/132.0
Accept: video/webm,video/ogg,video/*;q=0.9,application/ogg;q=0.7,audio/*;q=0.6,*/*;q=0.5
Accept-Language: ja
Range: bytes=0-
DNT: 1
Connection: keep-alive
Referer: https://hexe.net/2022/09/17/17:04:55/
Sec-Fetch-Dest: video
Sec-Fetch-Mode: no-cors
Sec-Fetch-Site: same-origin
Accept-Encoding: identity
Priority: u=4
TE: trailers

Response:

HTTP/2 206 
server: nginx/1.26.1
date: Sat, 16 Nov 2024 14:48:38 GMT
content-type: video/mp4
content-length: 65347058
accept-ranges: bytes
cache-control: public, max-age=0
last-modified: Sat, 17 Sep 2022 08:04:49 GMT
etag: W/"3e51df2-1834a7a8ef4"
content-range: bytes 0-65347057/65347058
content-disposition: inline; filename="eb3467a204907295921f367ac8187d67.original.mp4"
X-Firefox-Spdy: h2

I also checked in MS Edge 131.0.2903.51.

Checklist

mcollina
mcollina reviewed Nov 17, 2024
View reviewed changes
types/index.d.ts Outdated
/**
* Limit max response size when acceptRanges is true, defaults to the entire file size.
*/
maxChunkSize?: number | undefined;
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 please add a test for this type? We used tsd.

ledyba
ledyba commented Nov 17, 2024
View reviewed changes
Copy link
Author

@ledyba ledyba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I added tsd tests for maxChunkSize and acceptRanges.

Could you take another look?

@ledyba ledyba requested a review from mcollina November 17, 2024 10:42
@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 17, 2024

Is a maxChunkSize not basically a highWatermark? Should we name it like that?

@ledyba
Copy link
Author

ledyba commented Nov 17, 2024 •
edited
Loading

I don't think that maxChunkSize is the best name. I'm not an English native, so I'd appreciate any advice.

@Fdawgs Fdawgs requested a review from Copilot November 20, 2024 10:33
Copilot
Copilot AI reviewed Nov 20, 2024
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (2)

test/utils.js:12

  • The function name should be 'shouldHaveHeader' instead of 'shouldNotHaveHeader'.
module.exports.shouldHaveHeader = function shouldNotHaveHeader (header, t) {

README.md:52

  • The heading should be 'maxChunkSize' instead of 'acceptRanges'.
##### acceptRanges
Fdawgs
Fdawgs reviewed Nov 20, 2024
View reviewed changes
test/send.1.test.js Outdated Show resolved Hide resolved
Fdawgs
Fdawgs reviewed Nov 20, 2024
View reviewed changes
README.md Outdated
Comment on lines 52 to 56
##### acceptRanges

Specify maximum chunk size, defaults to send the entire file size.
This will be used when `acceptRanges` is true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be renamed to maxChunkSize and moved to be after maxAge, as the options are sorted alphabetically.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I also re-order the option in other files.

Fdawgs
Fdawgs reviewed Nov 20, 2024
View reviewed changes
test/utils.js Outdated Show resolved Hide resolved
@Fdawgs Fdawgs changed the title Add maxChunkSize option to SendOptions feat: add maxChunkSize option Nov 20, 2024
ledyba
ledyba commented Nov 21, 2024
View reviewed changes
Copy link
Author

@ledyba ledyba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review and typo suggestion!

Could you take another look?

README.md Outdated
Comment on lines 52 to 56
##### acceptRanges

Specify maximum chunk size, defaults to send the entire file size.
This will be used when `acceptRanges` is true.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I also re-order the option in other files.

@ledyba ledyba requested a review from Fdawgs November 21, 2024 12:41
Fdawgs
Fdawgs reviewed Nov 21, 2024
View reviewed changes
lib/send.js
const root = options.root
? resolve(options.root)
: null

return {
acceptRanges,
maxChunkSize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @ledyba, could you also move this to be in alphabetical order as well please?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 21, 2024

The name is not that nice. maxChunkSize is imho a highWatermark . @fastify/send only supports bytes for range. So it is de facto a highWaterMark

Uzlopak
Uzlopak reviewed Nov 21, 2024
View reviewed changes
lib/send.js
@@ -546,6 +551,9 @@ function sendFileDirectly (request, path, stat, options) {

// Content-Range
statusCode = 206
if (options.maxChunkSize) {
ranges[0].end = Math.min(ranges[0].end, ranges[0].start + options.maxChunkSize - 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesnt seem right.

Uzlopak
Uzlopak requested changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preemptive blocker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@Fdawgs Fdawgs Fdawgs left review comments

@Uzlopak Uzlopak Uzlopak requested changes

@mcollina mcollina Awaiting requested review from mcollina

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ledyba @Uzlopak @mcollina @Fdawgs