Skip to content

fix ref bug with iter, views and istr#1311

Open
Vizonex wants to merge 27 commits intoaio-libs:masterfrom
Vizonex:iterator-ref-leak
Open

fix ref bug with iter, views and istr#1311
Vizonex wants to merge 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).

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.

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