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 in upload session #377

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Conversation

beliaev-maksim
Copy link
Contributor

fix RAM leak in upload session #376
use file context manager to ensure file is closed
iterate using partial: https://docs.python.org/3/library/functions.html#iter

use file context manager to ensure file is closed
iterate using partial: https://docs.python.org/3/library/functions.html#iter
self._upload_results.append(upload_result)
elif f_pos + len(piece) < st.st_size:
upload_result = self.file.continue_upload(self._upload_id, f_pos, piece)
self._upload_results.append(upload_result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vgrem
also I am questioning the need to store the result. It is not used anywhere but consumes memory as well
or am I missing the place where it is utilized?

Copy link
Owner

@vgrem vgrem Aug 5, 2021

Choose a reason for hiding this comment

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

Greetings! First of all, thank you for catching it! Agree with you, the way how it is implemented in the current version, does not prevent the whole file to be read into memory.
That basically contradicts to the advantage of utilizing lazy load approach which was supposed to bring an efficiency in terms of memory consumption.

And last but not least, thank you for the proposal and PR, lets merge it and see if works as expected.

Copy link
Owner

Choose a reason for hiding this comment

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

speaking of storing of results, right call! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great, thank you!

@vgrem vgrem merged commit 5c1a9f5 into vgrem:master Aug 5, 2021
vgrem added a commit that referenced this pull request Aug 5, 2021
@beliaev-maksim beliaev-maksim deleted the mbeliaev/upload_session branch August 11, 2021 17:00
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.

2 participants