Skip to content

Conversation

@Vizonex
Copy link
Contributor

@Vizonex Vizonex commented Jul 27, 2025

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 MultiDict and CIMultiDict)

  • pop(...) & popone(...)

  • popall(...)

  • __delitem__(...) or del 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 MultiDict via 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

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Vizonex Vizonex requested a review from asvetlov as a code owner July 27, 2025 20:21
@Vizonex Vizonex marked this pull request as draft July 27, 2025 20:21
@Vizonex Vizonex changed the title Fix Memory Leak for While Deleting Values (Draft) Fix Memory Leak when deleting values (Draft) Jul 27, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Jul 27, 2025

CodSpeed Performance Report

Merging #1233 will not alter performance

Comparing Vizonex:fix-pop-memory-leak (53b7bb6) with master (00e3803)

Summary

✅ 245 untouched benchmarks

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 28, 2025
@Vizonex Vizonex changed the title Fix Memory Leak when deleting values (Draft) Add Memory Leak Test for deleting values from a Mulitidict to prevent Bougus Claims or Memory leaks related to deletion from passing. Jul 28, 2025
@Vizonex Vizonex changed the title Add Memory Leak Test for deleting values from a Mulitidict to prevent Bougus Claims or Memory leaks related to deletion from passing. Fix Memory leaks and add tests to prevent memory leaks during deletion from passing Jul 28, 2025
@Vizonex Vizonex marked this pull request as ready for review July 28, 2025 01:36
@Vizonex Vizonex requested a review from webknjaz as a code owner July 28, 2025 01:36
@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 7, 2025

@asvetlov When this passes or you decide to put this new fix into effect feel free to yank 6.6.2 & whatever the current version is for me since it leaks memory and I don't think users want to deal with that. I changed my mind Dreamsourcer explained to me that this is not possible, whelp the least people could do is update.

@codecov
Copy link

codecov bot commented Aug 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.85%. Comparing base (00e3803) to head (53b7bb6).
⚠️ Report is 27 commits behind head on master.

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           
Flag Coverage Δ
CI-GHA 99.85% <100.00%> (+<0.01%) ⬆️
pytest 99.85% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 7, 2025

is it ok if I tell code-coverage to ignore this line of code due to it's circumstances?
https://github.com/aio-libs/multidict/pull/1233/files#diff-0c90c5c5b57ca508e181f695abfdf1094c1cc261c27c8bfb0c6df5419efce9eaR34

 if usage > 50:
     print(f"Memory leaked at: {usage} MB")
     sys.exit(1)

@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 8, 2025

I think I'm going to change how md_clear is approched a bit later today by reverting some of it's functionality but making a new function or parameter for when dellocating and deletion happens. The parameter or new function should not be exposed to the CAPI that I & avestlov have been working on however as it should be tied directly to the dealloc function. Nevermind I think it's good enough. I just needed to change how code-coverage was handling weather or not memory leaked.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

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.

@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 10, 2025

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 md_clear

@asvetlov
Copy link
Member

Don't redo please but revert only changes in C source files that are not related to md_clear() implementation.

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.

@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 10, 2025

Don't redo please but revert only changes in C source files that are not related to md_clear() implementation.

@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.

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.
I see now that makes more sense to me. I'll get rid of my debugger for you. I was using to track where the memory leak originated from since it was more or less a puzzle to me until I figured out what I needed to fix.

If it is uncomfortable for you, I can tidy up the source myself.
No it's ok I can do it real quickly. I should be able capable of doing so.

@Vizonex Vizonex requested a review from asvetlov August 10, 2025 21:03
@Vizonex Vizonex changed the title Fix Memory leaks and add tests to prevent memory leaks during deletion from passing Fix Memory leaks and add tests to prevent memory leaks during md_clear from passing Aug 10, 2025
@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 10, 2025

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.

@asvetlov asvetlov merged commit 820631f into aio-libs:master Aug 11, 2025
64 checks passed
@asvetlov
Copy link
Member

Thank you, @Vizonex

@Vizonex Vizonex deleted the fix-pop-memory-leak branch August 11, 2025 14:14
@Vizonex
Copy link
Contributor Author

Vizonex commented Aug 11, 2025

@asvetlov You're welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak in CIMultiDict.pop() method?

2 participants