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

Use buffer instead of cloned stream #279

Merged
merged 3 commits into from May 1, 2022
Merged

Use buffer instead of cloned stream #279

merged 3 commits into from May 1, 2022

Conversation

MrOrz
Copy link
Member

@MrOrz MrOrz commented Apr 28, 2022

Current implementation of CreateMediaArticle will halt on file larger than a certain size.

It is halt because await file.clone().buffer() never resolves.

The root cause is that when we call await file.clone().buffer(), it reads the cloned stream and put its data in an internal buffer so that the original stream can still consume data afterwards. The read operation halts when the internal buffer is full.

Related issue:

Analysis

In the current implementation we tried to use stream so that we don't need to load the entire file in memory nor in file system.

However, image-hash package requires a buffer as its input. Even if we provide a file path to it, it will read the entire file into memory (then send to JPEG decoder) anyway. Therefore, it is inevitable that we put the entire file in memory.

Proposed fix

This PR downloads the entire file from mediaUrl into a buffer and then pass to image-hash.

We set a max limit for the downloaded file (5MB for now) so that the API server will not run out of memory when someone send us a super large file. If a file bigger than the limit is provided, CreateMediaArticle will fail.

@coveralls
Copy link

coveralls commented Apr 28, 2022

Coverage Status

Coverage decreased (-0.06%) to 87.473% when pulling f6cac74 on use-buffer into e0510ec on fix-build-by-upgrade.

@MrOrz MrOrz marked this pull request as ready for review April 28, 2022 13:37
@MrOrz MrOrz self-assigned this Apr 28, 2022
@MrOrz MrOrz requested a review from nonumpa April 28, 2022 13:37
Copy link
Member

@nonumpa nonumpa 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 for the fix 🙏

Base automatically changed from fix-build-by-upgrade to master May 1, 2022 15:26
@MrOrz MrOrz merged commit 271bc9b into master May 1, 2022
@MrOrz MrOrz deleted the use-buffer branch May 1, 2022 15:27
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.

None yet

3 participants