Skip to content

fix ref bug with iter, views and istr#1311

Merged
bdraco merged 27 commits intoaio-libs:masterfrom
Vizonex:iterator-ref-leak
Apr 18, 2026
Merged

fix ref bug with iter, views and istr#1311
bdraco merged 27 commits intoaio-libs:masterfrom
Vizonex:iterator-ref-leak

Conversation

@Vizonex
Copy link
Copy Markdown
Member

@Vizonex Vizonex commented Mar 28, 2026

What do these changes do?

This is based off previous things that I have referenced in #1310 and #1306. The other bugs listed seem to like much harder puzzles to solve than anything else so I'm tackling the easy fixable ones first.

Reproducer

"""Reproducer: multidict iterator/view/istr types leak type references.

iter.h:134 — multidict_iter_dealloc: no Py_DECREF(Py_TYPE(self))
views.h:30 — multidict_view_dealloc: no Py_DECREF(Py_TYPE(self))
istr.h:22 — istr_dealloc: no Py_DECREF(Py_TYPE(self))
traverse functions also missing Py_VISIT(Py_TYPE(self)).

Tested: multidict 6.7.1, Python 3.14.
"""
from multidict import MultiDict, istr
import sys

md = MultiDict([("a", "1"), ("b", "2")])

# Test iterator type leak
it = iter(md.keys())
iter_type = type(it)
del it
baseline = sys.getrefcount(iter_type)
for i in range(1000):
    it = iter(md.keys())
    list(it)
    del it
after = sys.getrefcount(iter_type)
print(f"KeysIter type: baseline={baseline}, after={after}, leaked={after-baseline}")

# Test view type leak
view_type = type(md.keys())
baseline = sys.getrefcount(view_type)
for i in range(1000):
    v = md.keys()
    del v
after = sys.getrefcount(view_type)
print(f"KeysView type: baseline={baseline}, after={after}, leaked={after-baseline}")

# Test istr type leak
baseline = sys.getrefcount(istr)
for i in range(1000):
    s = istr("hello")
    del s
after = sys.getrefcount(istr)
print(f"istr type:     baseline={baseline}, after={after}, leaked={after-baseline}")

# Expected: 0 leaked for all
# Actual: exactly 1000 leaked for each — one per object lifecycle

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

@Vizonex Vizonex requested a review from asvetlov as a code owner March 28, 2026 19:22
@Vizonex Vizonex requested a review from webknjaz as a code owner March 28, 2026 19:25
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Mar 28, 2026
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 29, 2026

Would you please add a test for this one as well ?

@Vizonex
Copy link
Copy Markdown
Member Author

Vizonex commented Mar 30, 2026

Would you please add a test for this one as well ?

Just did. :)

@Vizonex
Copy link
Copy Markdown
Member Author

Vizonex commented Mar 30, 2026

Seems this implementation likes to segfault :( I'll probably go back and replan a few things tomorrow when I have 2 days off from my job.

Vizonex added 2 commits March 31, 2026 12:45
…ypes after finishing and use a tp_free slot to help with that.
@Vizonex
Copy link
Copy Markdown
Member Author

Vizonex commented Mar 31, 2026

@bdraco I seem to have just fixed the problem. Turns out using a Py_tp_free can be very useful. I think what occurred was that the rewrite was modeled after the python standard library a little bit too deeply. I decided it might be smart to try another implementation such as freeing the type afterwards.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 31, 2026

Merging this PR will not alter performance

✅ 245 untouched benchmarks


Comparing Vizonex:iterator-ref-leak (c7d3d1d) with master (edb61e8)

Open in CodSpeed

@Vizonex
Copy link
Copy Markdown
Member Author

Vizonex commented Mar 31, 2026

The only bad news is that freethreaded is still bugged.

… instead and recast istr directly instead of using _PyObject_CAST
@Vizonex
Copy link
Copy Markdown
Member Author

Vizonex commented Mar 31, 2026

@bdraco It should also be mentioned that code-coverage is somehow failing in the isolated directory. I've looked at them and most files seem to act that way at around 33.33% or similar results. I wonder if this coverage is inaccurate or should be fixed?

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 31, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.86%. Comparing base (edb61e8) to head (c7d3d1d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1311   +/-   ##
=======================================
  Coverage   99.85%   99.86%           
=======================================
  Files          26       28    +2     
  Lines        3539     3607   +68     
  Branches      254      265   +11     
=======================================
+ Hits         3534     3602   +68     
  Misses          3        3           
  Partials        2        2           
Flag Coverage Δ
CI-GHA 99.86% <100.00%> (+<0.01%) ⬆️
pytest 99.86% <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.

Comment thread tests/isolated/multidict_type_leak.py Fixed
Comment thread tests/isolated/multidict_type_leak.py Fixed
Comment thread tests/isolated/multidict_type_leak.py Fixed
Comment thread tests/isolated/multidict_type_leak.py Fixed
Comment thread tests/isolated/multidict_type_leak.py Fixed
@Vizonex Vizonex mentioned this pull request Apr 3, 2026
3 tasks
Comment thread multidict/_multilib/istr.h Outdated
Comment thread tests/isolated/multidict_type_leak.py Dismissed
Comment thread tests/isolated/multidict_type_leak.py Dismissed
Comment thread tests/isolated/multidict_type_leak_items_values.py Dismissed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a CPython C-extension heap-type lifecycle issue in multidict where iterator/view/istr instances were leaking references to their types, and adds isolated regression tests to prevent recurrence.

Changes:

  • Add Py_DECREF(Py_TYPE(self)) handling in iterator/view/istr deallocators to balance per-instance heap-type references.
  • Add Py_VISIT(Py_TYPE(self)) to iterator/view traverse functions to properly participate in GC cycle detection.
  • Add isolated leak regression tests covering keys/items/values iterators & views plus istr, and register them in the leak test suite.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
multidict/_multilib/iter.h Fix iterator heap-type ref leak in tp_dealloc and add type visiting in tp_traverse.
multidict/_multilib/views.h Fix view heap-type ref leak in tp_dealloc and add type visiting in tp_traverse.
multidict/_multilib/istr.h Fix istr heap-type ref leak by balancing the type reference in tp_dealloc.
tests/test_leaks.py Register new isolated leak regression scripts in the existing pytest harness.
tests/isolated/multidict_type_leak.py Add isolated regression test for keys iterator/view types and istr type refcounts.
tests/isolated/multidict_type_leak_items_values.py Add isolated regression tests for items/values iterator/view type refcounts.
CHANGES/1311.bugfix.rst Add changelog entry for the fixed reference leak.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bdraco bdraco merged commit b9bf25c into aio-libs:master Apr 18, 2026
70 of 71 checks passed
@Vizonex Vizonex deleted the iterator-ref-leak branch April 18, 2026 21:27
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.

4 participants