Skip to content

use streams instead of buffers #12099

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
Merged
merged 8 commits into from
Feb 23, 2022
Merged

use streams instead of buffers #12099

merged 8 commits into from
Feb 23, 2022

Conversation

@petersg83
Copy link
Contributor

@petersg83 petersg83 commented Jan 4, 2022 •
edited
Loading

Done:

  • use streams instead of buffers

Should fix #6432

@codecov
Copy link

codecov bot commented Jan 4, 2022 •
edited
Loading

Codecov Report

Merging #12099 (7fbe76d) into master (42f1727) will decrease coverage by 0.10%.
The diff coverage is 7.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12099      +/-   ##
==========================================
- Coverage   47.75%   47.64%   -0.11%     
==========================================
  Files         228      228              
  Lines        8446     8473      +27     
  Branches     1884     1891       +7     
==========================================
+ Hits         4033     4037       +4     
- Misses       3629     3648      +19     
- Partials      784      788       +4
Flag Coverage Δ
front ?
unit 47.64% <7.69%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/upload/server/services/upload.js 16.38% <6.75%> (+0.21%) ⬆️
packages/providers/upload-local/lib/index.js 23.68% <10.00%> (-4.89%) ⬇️
packages/core/upload/server/utils/file.js 23.52% <14.28%> (-6.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42f1727...7fbe76d. Read the comment docs.

petersg83
petersg83 commented Jan 4, 2022
View reviewed changes
alexandrebodin
alexandrebodin reviewed Jan 5, 2022
View reviewed changes
@alexandrebodin alexandrebodin added issue: enhancement Issue suggesting an enhancement to an existing feature source: core:upload Source is core/upload package labels Jan 5, 2022
@alexandrebodin alexandrebodin modified the milestones: 4.0.3, 4.0.4 Jan 5, 2022
@derrickmehaffy derrickmehaffy linked an issue Jan 5, 2022 that may be closed by this pull request
Timeout error while uploading large files over S3 #9722
Closed
@derrickmehaffy derrickmehaffy mentioned this pull request Jan 5, 2022
Closed
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from 4598b38 to 97af2ac Compare January 6, 2022 13:30
@petersg83 petersg83 marked this pull request as ready for review January 7, 2022 17:16
@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/discussion-regarding-strapi-provider-upload-rackspace/14274/1

gu-stav
gu-stav reviewed Jan 8, 2022
View reviewed changes
@alexandrebodin alexandrebodin modified the milestones: 4.0.4, 4.0.5 Jan 10, 2022
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from 9f0e4ea to 082d4ab Compare January 17, 2022 09:29
@alexandrebodin alexandrebodin modified the milestones: 4.0.5, 4.0.6 Jan 19, 2022
@Convly Convly modified the milestones: 4.0.6, 4.0.7, 4.0.8 Jan 28, 2022
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from 082d4ab to 98605b1 Compare February 7, 2022 15:35
@petersg83 petersg83 added the flag: don't merge This PR should not be merged at the moment label Feb 8, 2022
alexandrebodin
alexandrebodin requested changes Feb 14, 2022
View reviewed changes
@petersg83 petersg83 added the flag: don't merge This PR should not be merged at the moment label Feb 15, 2022
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from 12dfebd to d92e0b4 Compare February 16, 2022 11:23
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from d92e0b4 to d22fb53 Compare February 16, 2022 13:49
@alexandrebodin alexandrebodin removed this from the 4.0.8 milestone Feb 16, 2022
@petersg83 petersg83 added this to the 4.1.0 milestone Feb 16, 2022
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from d22fb53 to 7ad9af5 Compare February 16, 2022 14:35
@alexandrebodin alexandrebodin modified the milestones: 4.1.0, 4.1.1 Feb 17, 2022
@petersg83 petersg83 mentioned this pull request Feb 17, 2022
Merged
@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from 7ad9af5 to ef1e2b1 Compare February 18, 2022 16:23
@petersg83 petersg83 removed the flag: don't merge This PR should not be merged at the moment label Feb 22, 2022
@alexandrebodin alexandrebodin self-assigned this Feb 23, 2022
alexandrebodin
alexandrebodin previously approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM. Great improvements!

@petersg83 petersg83 force-pushed the fix/medialib-memory-leak branch from ef1e2b1 to 83a5aeb Compare February 23, 2022 10:42
@alexandrebodin alexandrebodin merged commit d81eebf into master Feb 23, 2022
@alexandrebodin alexandrebodin deleted the fix/medialib-memory-leak branch February 23, 2022 12:43
@pedrosimao
Copy link

Yey! Thanks a lot @petersg83 !

@rktyt rktyt mentioned this pull request Feb 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexandrebodin alexandrebodin alexandrebodin approved these changes

@Elhebert Elhebert Awaiting requested review from Elhebert

@derrickmehaffy derrickmehaffy Awaiting requested review from derrickmehaffy

@Convly Convly Awaiting requested review from Convly

@gu-stav gu-stav Awaiting requested review from gu-stav

Assignees

@alexandrebodin alexandrebodin

Labels

issue: enhancement Issue suggesting an enhancement to an existing feature source: core:upload Source is core/upload package

Projects

None yet

Milestone

4.1.1

Development

Successfully merging this pull request may close these issues.

Timeout error while uploading large files over S3 Multiple Image Uploads result in memory leak

9 participants

@petersg83 @strapi-bot @pedrosimao @gu-stav @Tragio @alexandrebodin @derrickmehaffy @Convly