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 memory hole while uploading 5G chunks #620

Closed
wants to merge 1 commit into from

Conversation

josvazg
Copy link

@josvazg josvazg commented Feb 5, 2017

The call read(MAX_FILE_SIZE) will load the hole MAX_FILE_SIZE into
memory, which turns to be 5GB. That puts the system to a crawl.

Instead, the better way to go around this is to use streaming, copying
small chunks of memory in a loop until all bytes are transferred.
Python provides shutil.copyfileobj that saves you even writting the loop
explicitly.

Apart from that:

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 96.704% when pulling b0356be on josvazg:fix/stream_copy into e7d729f on rackspace:working.

@josvazg josvazg force-pushed the fix/stream_copy branch 2 times, most recently from 8f71c5a to 8c9dc3e Compare February 5, 2017 15:03
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.709% when pulling 8c9dc3e on josvazg:fix/stream_copy into e7d729f on rackspace:working.

@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage increased (+0.04%) to 96.709% when pulling 8c9dc3e on josvazg:fix/stream_copy into e7d729f on rackspace:working.

The call read(MAX_FILE_SIZE) will load the hole MAX_FILE_SIZE into
memory, which turns to be 5GB. That puts the system to a crawl.

Instead, the better way to go around this is to use streaming, copying
small chunks of memory in a loop until all bytes are transferred.

Apart from that:
- xreadlines has been deprecated for quite a while now:
  https://www.quantifiedcode.com/knowledge-base/maintainability/%60.xreadlines()%60%20deprecated/7lC2NACa
- Tox was not able to run pypy test env properly without the explicit
  dependencies on requests and nose.

Includes unitests for copy_maxlen_bytes:

- One test for the typical case; several writes, last one shorter
- EOF test; copy ends before maxlen due to EOF reached first
- Exact copies; when bufsize is a exact divisor of maxlen
- One copy; when maxlen is less than bufsize
@coveralls
Copy link

coveralls commented Feb 5, 2017

Coverage Status

Coverage increased (+0.04%) to 96.709% when pulling 6f797f5 on josvazg:fix/stream_copy into e7d729f on rackspace:working.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 96.709% when pulling 4909b33 on josvazg:fix/stream_copy into e7d729f on rackspace:working.

@briancurtin
Copy link
Contributor

We're currently phasing out Pyrax in favor of openstacksdk/rackspacesdk and are not currently accepting changes like this.

@briancurtin briancurtin closed this Feb 6, 2017
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.

3 participants