Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

call PyObject_GC_Untrack before deallocating #3404

Merged
merged 4 commits into from
Sep 11, 2023
Merged

call PyObject_GC_Untrack before deallocating #3404

merged 4 commits into from
Sep 11, 2023

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Aug 18, 2023 •
edited
Loading

Fixes #3064
Closes #3416

I verified by hand that this fixes the warning on a debug build of Python. As we don't run debug builds in CI I'm unsure if there's any way we can test this. EDIT: now testing debug build in CI as part of this PR.

As a bit of refactoring here I removed Layout from PyClassImpl as it's no longer necessary.

davidhewitt
davidhewitt commented Aug 18, 2023
View reviewed changes
noxfile.py Show resolved Hide resolved
src/impl_/freelist.rs Show resolved Hide resolved
src/pyclass/create_type_object.rs Show resolved Hide resolved
@davidhewitt davidhewitt mentioned this pull request Aug 18, 2023
Merged
@davidhewitt davidhewitt added this pull request to the merge queue Aug 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 20, 2023
@davidhewitt davidhewitt mentioned this pull request Aug 26, 2023
Closed
@davidhewitt davidhewitt force-pushed the fix-dealloc branch 2 times, most recently from 843ceaf to aac7d99 Compare September 6, 2023 06:29
@davidhewitt
Copy link
Member Author

With the test-debug CI run in #3430 we now can reproduce the resourcewarning lines from #3064, e.g. in https://github.com/PyO3/pyo3/actions/runs/6093238722/job/16532577116#step:9:1196

I've rebased on #3430 here and it's great to see that those lines are gone and even better, test-debug is now fixed.

@davidhewitt davidhewitt mentioned this pull request Sep 6, 2023
Closed
@davidhewitt davidhewitt added this pull request to the merge queue Sep 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 8, 2023
@davidhewitt davidhewitt added CI-no-fail-fast If one job fails, allow the rest to keep testing CI-build-full labels Sep 9, 2023
@davidhewitt
Copy link
Member Author

Looks like everything except 3.11 and pypy is broken 😅

@davidhewitt
Copy link
Member Author

Ok, think I've fixed the issues. The 3.12 failures were #3444, and the 3.10 (and older) cpython failures were due to an annoying assumption in BaseException_dealloc that the object was still tracked, see the last commit pushed. Will proceed to merge if this really does go green...

@davidhewitt davidhewitt added this pull request to the merge queue Sep 11, 2023
Merged via the queue into PyO3:main with commit bcb0104 Sep 11, 2023
@davidhewitt davidhewitt deleted the fix-dealloc branch September 11, 2023 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamreichold adamreichold adamreichold approved these changes

@messense messense messense approved these changes

Assignees
No one assigned
Labels
CI-build-full CI-no-fail-fast If one job fails, allow the rest to keep testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run CI with Python 3.11 debug build ResourceWarning: Object of type XXX is not untracked before destruction
3 participants
@davidhewitt @messense @adamreichold