-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 MultipartReader for big files #4865
Merged
Merged
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
8bee16a
Fix max memory to be 1mb
marcosc90 e75b476
Copy the correct amount of bytes to temp file
marcosc90 472f8f7
Allow multipartReader to handle files above buffer max size
marcosc90 b956eca
Check against maxMemory instead of maxValueBytes
marcosc90 509dce0
Add test to check that a big file is written completely to disk
marcosc90 2fb40c0
Revert "Fix max memory to be 1mb"
marcosc90 e686be2
Fix readForm maxMemory comment to match implementation
marcosc90 4485e86
Merge branch 'master' into fix-multipart-reader
ry 052bf84
Change copy argument order (#4885)
marcosc90 5704fed
Pass object to Deno.open
marcosc90 b532498
Merge branch 'master' into fix-multipart-reader
marcosc90 fae1d77
Merge branch 'master' into fix-multipart-reader
marcosc90 9b6b98b
Improve test
marcosc90 79b0b09
Merge branch 'master' into fix-multipart-reader
marcosc90 cdd7856
Use copy with MultiReader instead of copyN & copy
marcosc90 669311a
Don't swallow error if copy fails
marcosc90 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -281,10 +281,10 @@ export class MultipartReader { | |
* null value means parsing or writing to file was failed in some reason. | ||
* @param maxMemory maximum memory size to store file in memory. bytes. @default 1048576 (1MB) | ||
* */ | ||
async readForm(maxMemory = 10 << 20): Promise<MultipartFormData> { | ||
async readForm(maxMemory = 1 << 20): Promise<MultipartFormData> { | ||
const fileMap = new Map<string, FormFile>(); | ||
const valueMap = new Map<string, string>(); | ||
let maxValueBytes = maxMemory + (10 << 20); | ||
let maxValueBytes = maxMemory + (1 << 20); | ||
const buf = new Buffer(new Uint8Array(maxValueBytes)); | ||
for (;;) { | ||
const p = await this.nextPart(); | ||
|
@@ -308,7 +308,7 @@ export class MultipartReader { | |
} | ||
// file | ||
let formFile: FormFile | undefined; | ||
const n = await copy(buf, p); | ||
const n = await copyN(buf, p, maxValueBytes); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While testing I noticed that when parsing big files, it was crashing with: This happens because |
||
const contentType = p.headers.get("content-type"); | ||
assert(contentType != null, "content-type must be set"); | ||
if (n > maxMemory) { | ||
|
@@ -319,11 +319,11 @@ export class MultipartReader { | |
postfix: ext, | ||
}); | ||
try { | ||
const size = await copyN( | ||
file, | ||
new MultiReader(buf, p), | ||
maxValueBytes | ||
); | ||
// write buffer to file | ||
let size = await copyN(file, buf, n); | ||
// Write the rest of the file | ||
size += await copy(file, new MultiReader(buf, p)); | ||
|
||
file.close(); | ||
formFile = { | ||
filename: p.fileName, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@ry
go
uses10MB
as default inreadForm
. Yet the comment in the code says1MB
, just want confirmation. I think1MB
it's a good value to keep in Buffer.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.
We should definitely follow Go's example. 10MB sounds good to me.
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.
Done. Reverted back to
10MB
and updated comment.