pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

avoid creating `PyRef` inside `__traverse__` handler

Open davidhewitt opened this issue 1 year ago • 4 comments

See https://github.com/PyO3/pyo3/issues/4265#issuecomment-2305890021

We cannot use PyRef<T> to guard access to the value inside a __traverse__ handler. Instead I use the PyClassObject<T> and manually perform the borrow checking.

davidhewitt avatar Aug 23 '24 22:08 davidhewitt

cc @ngoldbaum

davidhewitt avatar Aug 23 '24 22:08 davidhewitt

We could also make the change suggested by the comment here....

#[repr(transparent)]
pub struct PyRef<'p, T: PyClass> {
    // TODO: once the GIL Ref API is removed, consider adding a lifetime parameter to `PyRef` to
    // store `Borrowed` here instead, avoiding reference counting overhead.
    inner: Bound<'p, T>,
}

as that would also avoid changing reference counts.

mejrs avatar Aug 24 '24 07:08 mejrs

I believe that would need #4390. Also in that thread https://github.com/PyO3/pyo3/pull/4390#discussion_r1702907653 there was the suggestion to maybe delay that change to 0.24

Icxolu avatar Aug 24 '24 08:08 Icxolu

Yes agreed, I considered changing PyRef but would prefer to delay given there's enough breaking changes coming in 0.23. I also want to be careful with changing PyRef right now because I think it might need other changes for the freethreaded build.

I also think I might backport this fix to 0.22, which would be another reason not to have the breaking change here.

davidhewitt avatar Aug 24 '24 09:08 davidhewitt

I did some local testing on this PR.

After rebasing on main to fix some noisy tests that were fixed by me last week, this does seem to fix the "accessing the GIL while a __traverse__ implementation is running" panics.

ngoldbaum avatar Aug 28 '24 16:08 ngoldbaum

Great, will merge this. I am considering landing this as part of a stack of fixes in a 0.22.3 patch.

davidhewitt avatar Aug 28 '24 21:08 davidhewitt