Skip to content

Fix memory leak caused by identity when default value is inplace#1284

Merged
bdraco merged 14 commits intoaio-libs:masterfrom
Vizonex:pop-memory-leak-fix
Jan 25, 2026
Merged

Fix memory leak caused by identity when default value is inplace#1284
bdraco merged 14 commits intoaio-libs:masterfrom
Vizonex:pop-memory-leak-fix

Conversation

@Vizonex
Copy link
Member

@Vizonex Vizonex commented Jan 22, 2026 •
edited
Loading

What do these changes do?

This was reported a while ago without a reproducer to use until @rodrigobnogueira provided me some very detailed information about where the problem was and how to fix it. So I went ahead and added in something to ensure that the identity value is properly handled if or when the user has provided a default value to use.

Are there changes in behavior for the user?

A Slow memory leak should be patched now.

Related issue number

fixes #1273

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 January 22, 2026 19:42
@Vizonex Vizonex requested a review from webknjaz as a code owner January 22, 2026 19:46
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 22, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 22, 2026 •
edited
Loading

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing Vizonex:pop-memory-leak-fix (072b492) with master (87dd4a4)

Summary

✅ 245 untouched benchmarks

@Vizonex Vizonex mentioned this pull request Jan 22, 2026
Closed
webknjaz
webknjaz reviewed Jan 22, 2026
View reviewed changes
rodrigobnogueira
rodrigobnogueira approved these changes Jan 22, 2026
View reviewed changes
Copy link
Member

@rodrigobnogueira rodrigobnogueira left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The fix looks good. I'd add a test for it. thanks

bdraco
bdraco approved these changes Jan 22, 2026
View reviewed changes
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Fix looks correct. Note there are still other review comments

@bdraco
Copy link
Member

bdraco commented Jan 22, 2026

Thanks @Vizonex

@bdraco
Copy link
Member

bdraco commented Jan 22, 2026

I'll see if I can get the CI unblocked

bdraco and others added 3 commits January 22, 2026 13:48
@bdraco
Merge branch 'master' into pop-memory-leak-fix
545d245
@Vizonex @webknjaz
Update CHANGES/1284.bugfix.rst
e0e4c12 
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
@Vizonex
Merge branch 'master' into pop-memory-leak-fix
02edfa3
@Vizonex
Copy link
Member Author

Vizonex commented Jan 23, 2026 •
edited
Loading

The fix looks good. I'd add a test for it. thanks

@rodrigobnogueira Feel free to do so ^^ I'm sure you can figure out how to fork my fork add in the test and then send the pull request to my end. It will same me a bit of time.

@Vizonex
Copy link
Member Author

Vizonex commented Jan 23, 2026

@rodrigobnogueira Feel free to let me know if the test should use bigger numbers or something more intense. I kept everything rather minimal because these tests are on a timer if I remember correctly.

@codecov
Copy link

codecov bot commented Jan 23, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.85%. Comparing base (87dd4a4) to head (072b492).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1284   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          26       26           
  Lines        3507     3513    +6     
  Branches      252      253    +1     
=======================================
+ Hits         3502     3508    +6     
  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.

@bdraco
Copy link
Member

bdraco commented Jan 24, 2026

I'll merge and do a release on Sunday or Monday if there are no other comments before than

rodrigobnogueira
rodrigobnogueira reviewed Jan 24, 2026
View reviewed changes
rodrigobnogueira
rodrigobnogueira suggested changes Jan 24, 2026
View reviewed changes
bdraco
bdraco approved these changes Jan 25, 2026
View reviewed changes
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Thanks @Vizonex

@bdraco bdraco merged commit 77bb95e into aio-libs:master Jan 25, 2026
78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdraco bdraco bdraco approved these changes

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

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

+1 more reviewer

@rodrigobnogueira rodrigobnogueira rodrigobnogueira approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory Leak on 6.6.4 CIMultiDict pop

4 participants

@Vizonex @bdraco @webknjaz @rodrigobnogueira