Fix Memory leaks and add tests to prevent memory leaks during md_clear from passing#1233
Conversation
for more information, see https://pre-commit.ci
CodSpeed Performance ReportMerging #1233 will not alter performanceComparing Summary
|
…s claims can be prevented. add types-psutil to pre-commit for typechecking the test
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1233 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 25 26 +1
Lines 3459 3510 +51
Branches 239 252 +13
=======================================
+ Hits 3454 3505 +51
Misses 3 3
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
is it ok if I tell code-coverage to ignore this line of code due to it's circumstances? if usage > 50:
print(f"Memory leaked at: {usage} MB")
sys.exit(1) |
|
|
…values are being cleared
asvetlov
left a comment
There was a problem hiding this comment.
Thanks for the patch!
Your fix is correct.
However, I'd like to ask you to revert all changes in .c files except the md_clear() function body.
They make a mess in git history without actually fixing anything.
I apologize for that I didn't know what else should be done. I am wondering if I should redo the pull request or just cleanup everything except for |
|
Don't redo please but revert only changes in C source files that are not related to Maybe I was not clear about git history concern. We squash pull requests before merging, so the number of commits doesn't matter. What I don't want to see are artefacts of your debugging sessions: commented out debugger helper function, meaningless replacements of Py_DECREF with Py_XDECREF etc. If it is uncomfortable for you, I can tidy up the source myself. |
@asvetlov Ok I can do that. That shouldn't be a problem for me to take care of the one fear I had was reverting changes if a new memory-leak happened but I will take you at your word this time.
|
|
I cleaned up the artifacts now I will wait. I'm not worried if this takes any longer because I already let users who are aware of this issue know that they can temporarily use this fork I'm trying to merge until multidict has a new release. |
|
Thank you, @Vizonex |
|
@asvetlov You're welcome :) |
What do these changes do?
@F1int0m had discovered a memory leak related to the following functions in the C extension of multidict, all the following functions are affected (This goes for both
MultiDictandCIMultiDict)pop(...)&popone(...)popall(...)__delitem__(...)ordel item[...]surprisingly
popitem(...)is not affected so I'm pretty sure it's a ref-count bug where a there's more refs being made than needed. But having gone through the code I have not found the bug yet. However I think all versions after the Hashtable was introduced (6.6.0+) are affected.Reason this is a draft and not a pull request is that the bug has not been found however to prevent future bugs like this one I have added an isolated test to test for memory-leaks upon the deletion of values. However this bug is pretty sneaky and could make it's way into aiohttp under conditions where the user is modifying a
MultiDictvia deletion of keys.Feel free to help me change any code as I go along attempting to tweak any C Functions or if you think the isolated-test I've added should use a more intense stress-test.
Edit: Based on trying the test with trimming memory. This claim mentioned above might be bogus. However I shall give credit where due and say that having this test should prevent bougus claims about deletion of items casuing memory leaks from appearing in the future.Edit 2: I stand corrected this is a problem.
Are there changes in behavior for the user?
Memory Leaks Related to item-deletion should not be allowed to pass now thanks to these changes which means that we will no longer have bogus claims about such things.
Related issue number
Checklist