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: copy buffer view when iterating on file #10

Merged
merged 4 commits into from
May 29, 2021
Merged

fix: copy buffer view when iterating on file #10

merged 4 commits into from
May 29, 2021

Conversation

lowlighter
Copy link
Contributor

Closes #8

As @lucacasonato debugged in denoland/deno#10467, scrambled response output on large files may occur because the buffer is not copied so its content may change between iterations

Unless it has changed, this module is used by deno deploy and thus causing problems on it when trying to load large local files (current workaround was to use raw.githubusercontent instead of fetching local files)

It requires the patch for - writable: true from #9 to work, I left it out to avoid conflicts with it 👍

@lowlighter
Copy link
Contributor Author

Seems that Default size of the buffer is 32kB., but added text file only weight 3kb so test probably doesn't cover fix. Should I duplicate it 11 times more ?

@lucacasonato
Copy link
Owner

Yeah please - or just generate a file into /tmp during the test.

@lowlighter
Copy link
Contributor Author

Let me know when #9 gets merged so I can rebase on it to make build successful 👍

@lucacasonato
Copy link
Owner

Merged :-)

@lowlighter
Copy link
Contributor Author

Should work now 🙂

Copy link
Owner

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks :-)

@lucacasonato lucacasonato merged commit ec4e691 into lucacasonato:main May 29, 2021
@lowlighter
Copy link
Contributor Author

Thanks a lot!
It may need a new release so denoland/deployctl can be patched too

My memory is a bit fuzzy about whether it only impacted deployctl or deno deploy too but if the later also uses this polyfill it may also need to be updated

@lucacasonato
Copy link
Owner

released 0.2.0 now. Deno Deploy does not use this polyfill.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fetching local file returns a scrambled output
2 participants