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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃惤 Fix memory leak in chunked streams handling code (#3631) #3635

Merged
merged 1 commit into from Mar 26, 2019
Merged

馃惤 Fix memory leak in chunked streams handling code (#3631) #3635

merged 1 commit into from Mar 26, 2019

Conversation

socketpair
Copy link
Contributor

@socketpair socketpair commented Mar 3, 2019 •
edited

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@socketpair socketpair mentioned this pull request Mar 3, 2019
Closed
2 tasks
@codecov-io
Copy link

codecov-io commented Mar 3, 2019 •
edited

Codecov Report

Merging #3635 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3635      +/-   ##
==========================================
- Coverage    97.9%   97.88%   -0.03%     
==========================================
  Files          43       43              
  Lines        8556     8559       +3     
  Branches     1376     1377       +1     
==========================================
+ Hits         8377     8378       +1     
- Misses         74       75       +1     
- Partials      105      106       +1
Impacted Files Coverage 螖
aiohttp/streams.py 98.2% <100%> (-0.51%) 猬囷笍

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 7345ffb...4c526ab. Read the comment docs.

@socketpair
Copy link
Contributor Author

@mvanderkroon could you please check if this fix helps ? if yes, I will merge and ask for backport.

@mvanderkroon
Copy link

@socketpair I can confirm that this fixes it, cheers!

@socketpair
Copy link
Contributor Author

socketpair commented Mar 4, 2019 •
edited

@asvetlov Please merge. And also backport to 3.5. This is important fix.

mnacharov
mnacharov suggested changes Mar 4, 2019
View reviewed changes
aiohttp/streams.py
# Prevent memory leak: drop useless chunk splits
while chunk_splits and chunk_splits[0] < self._cursor:
chunk_splits.pop(0)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to put this code to end_http_chunk_receiving function? This code is useful only for chunked responses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I will check slightly later

Copy link
Contributor Author

@socketpair socketpair Mar 4, 2019 •
edited

Choose a reason for hiding this comment

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

Yes, you are right, but we should trim unused chunk_splits as soon as possible. Suppose many chunk splits in the buffer, including actually last chunk. In your case we will not free that memory until GC collected the stream. In my case we will free splits during content consumption.

@socketpair
Copy link
Contributor Author

@asvetlov please review and merge

@socketpair
Copy link
Contributor Author

@kxepal @fafhrd91 I would merge, but review is required.

@asvetlov
Copy link
Member

The tests absence worries me.
Suppose we will rewrite the code eventually.
How to make sure that the bug will no return back?

@fafhrd91
Copy link
Member

I don鈥檛 do python anymore, my python got too rusty. Do not rely on my

@socketpair
Copy link
Contributor Author

@asvetlov I have added tests... But they check contents of private variable....

@socketpair
Copy link
Contributor Author

@kxepal @asvetlov ping

@kxepal
Copy link
Member

kxepal commented Mar 13, 2019

@socketpair
Testing privates is not good indeed but not always. I think it's fine while test description will clearly reference to the case. During refactoring / privates changes we will hit such tests and recover these knowledge bits.

On the other hand, how hard would be to have test against memory leak itself? That one could be useful for all the times.

@socketpair
Copy link
Contributor Author

Eventually, I tried different methods to check memory usage of Python, and did not find any stable one. So, please approve. I will try to rewrite test in near future through measuring of memory consumption.

@socketpair
Copy link
Contributor Author

@kxepal I have done it!

@socketpair
Copy link
Contributor Author

@kxepal @asvetlov Please review

@socketpair
Copy link
Contributor Author

@asvetlov @kxepal I have checked, this new test fails without my changes in aiohttp code . Please review and merge if everything is OK.

kxepal
kxepal approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@kxepal kxepal left a comment

Choose a reason for hiding this comment

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

@socketpair
Sorry for delay, busy times /: LGFM. Memleak test is great! I can reproduce issue with it and the fix.

@socketpair socketpair merged commit d52a2de into master Mar 26, 2019
@lock lock bot added the outdated label Mar 27, 2020
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mnacharov mnacharov mnacharov requested changes

@kxepal kxepal kxepal approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@socketpair @codecov-io @mvanderkroon @asvetlov @fafhrd91 @kxepal @mnacharov