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

Truncate SpooledTemporaryFile after flushing the buffer #169

Closed
wants to merge 3 commits into from

Conversation

jiinus
Copy link

@jiinus jiinus commented Jul 8, 2016

When uploading a file in chunks with S3BotoStorageFile I noticed that the underlying SpooledTemporaryFile is never truncated but instead successive write-calls just keep appending the file.

With each flush the file is seeked to the beginning though which results in malformed data on S3 as the contents of the previous flushes are uploaded again and again on each write.

Also added the missing S3.py. License seems to permit it?

@codecov-io
Copy link

codecov-io commented Jul 8, 2016

Codecov Report

Merging #169 into master will increase coverage by -35.73%.

@@            Coverage Diff             @@
##           master    #169       +/-   ##
==========================================
- Coverage   59.32%   23.6%   -35.73%     
==========================================
  Files          17      18        +1     
  Lines        1694    1716       +22     
==========================================
- Hits         1005     405      -600     
- Misses        689    1311      +622
Impacted Files Coverage Δ
storages/S3.py 0% <ø> (ø)
storages/backends/s3boto.py 86.37% <100%> (+0.24%)
storages/backends/ftp.py 0% <ø> (-87.62%)
storages/backends/sftpstorage.py 0% <ø> (-86.19%)
storages/backends/dropbox.py 91.93% <ø> (-5.44%)
storages/backends/hashpath.py 81.81% <ø> (-3.9%)
storages/backends/gs.py 70.58% <ø> (-0.85%)
storages/backends/symlinkorcopy.py 0% <ø> (ø)
storages/backends/mogile.py 0% <ø> (ø)
... and 10 more

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 72cd5d6...c74af2f. Read the comment docs.

@jiinus
Copy link
Author

jiinus commented Jul 8, 2016

This should also fix the problem mentioned in issue #160

@jschneier
Copy link
Owner

Thanks to you and @iceseyes for bringing this issue up. I'm going to write a failing test with a 5MB file and get this one fixed asap, probably with your fix.

I intentionally removed the S3.py file because it was deprecated in the original repo and I wanted to reduce my own maintenance burden, especially of already deprecated pieces of the library. New fork yadda yadda.

@m-barthelemy
Copy link

Hi there,

This is affecting us badly as well (from a 45MB file we end up with >15GB on S3 before Django give up).
Is there anything that could be done to help getting this fixed?

Thanks!

@todofixthis
Copy link

@danpalmer
Copy link

Hi, ping on this? We have just encountered an issue where uploading ~10MB of data resulted in ~20GB of data being written to S3 - it looks like this PR is the fix we need. Any possibility of a merge?

jnm added a commit to jnm/django-storages that referenced this pull request Jun 4, 2018
by truncating the buffer after uploading it. Follows the approach of jschneier#169.
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.

6 participants