fix ref bug with iter, views and istr#1311
Conversation
|
Would you please add a test for this one as well ? |
Just did. :) |
|
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. |
…ypes after finishing and use a tp_free slot to help with that.
|
@bdraco I seem to have just fixed the problem. Turns out using a |
|
The only bad news is that freethreaded is still bugged. |
… instead and recast istr directly instead of using _PyObject_CAST
|
@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 Report✅ All modified and coverable lines are covered by tests. 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
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:
|
There was a problem hiding this comment.
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/istrdeallocators 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.
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
Are there changes in behavior for the user?
Related issue number
Checklist