Raising warning inside `__traverse__` causes a segfault

[mejrs]

Code:

use pyo3::prelude::*;
use pyo3::py_run;
use pyo3::{PyTraverseError, PyVisit};

#[pyclass]
struct GcIntegration {
    self_ref: PyObject,
}

#[pymethods]
impl GcIntegration {
    fn __traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
        Python::with_gil(|py| {
            let warn = py.import("warnings").unwrap().getattr("warn").unwrap();
            warn.call1(("blah",)).unwrap();
        });

        visit.call(&self.self_ref)
    }

    fn __clear__(&mut self) {
        let gil = Python::acquire_gil();
        self.self_ref = gil.python().None();
    }
}

fn main() {
    Python::with_gil(|py| {
        let inst = PyCell::new(
            py,
            GcIntegration {
                self_ref: py.None(),
            },
        )
        .unwrap();

        let mut borrow = inst.borrow_mut();
        borrow.self_ref = inst.to_object(py);

        py_run!(py, inst, "import gc; assert inst in gc.get_objects()");
    });

    Python::with_gil(|py| {
        py.run("import gc; gc.collect()", None, None).unwrap();
    });
}

Results in:

error: process didn't exit successfully: `target\debug\my_module.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

Meta:

rustc 1.62.0-nightly (1f7fb6413 2022-04-10)
binary: rustc
commit-hash: 1f7fb6413d6d6c0c929b223e478e44c3db991b03
commit-date: 2022-04-10
host: x86_64-pc-windows-msvc
release: 1.62.0-nightly
LLVM version: 14.0.0

Python 3.10.2 (tags/v3.10.2:a58ebcc, Jan 17 2022, 14:12:15) [MSC v.1929 64 bit (AMD64)]

[davidhewitt]

Yeah, ouch 🤕

I'm not sure there's anything we can do about it other than carefully document that __traverse__ is very limited? I think it's not really intended for any Python APIs to be called in it.

[birkenfeld]

Should be unsafe to implement then.

[davidhewitt]

While I don't disagree, there isn't a syntax for this. unsafe impl is only for traits (so doesn't apply to #[pymethods]), and unsafe fn is for unsafe-to-call.

[birkenfeld]

Requiring unsafe fn traverse (even if it's not called from user code) is probably good enough.

[mejrs]

Do we know why this segfaults? I can imagine why, but I glanced through the capi documentation and didn't see it called out anywhere.

While I don't disagree, there isn't a syntax for this. unsafe impl is only for traits

You can have

#[pymethods]
unsafe impl Foo {
    // ...
}

This is syntactically correct but not semantically, so if the unsafe keyword is removed by the macro it will work.

[mejrs]

I'd like something better than slapping unsafe on it and calling it a day.

What about making Python::with_gil and Python::acquire_gil panic (more likely: abort) if called from within __traverse__? We can temporarily store usize::MAX inside the GIL_COUNT thread_local, and make those two functions check for it.

[birkenfeld]

Alternatively, stop modeling __traverse__ as a freely implementable method, and make it a macro with declarative syntax, where you just give the self members that should be visited. (Same would probably apply for __clear__.)

Looking at CPython sources, this should cover 90% of __traverse__ implementations, and the others can be done via an unsafe escape hatch.

[davidhewitt]

I was considering similar; I once experimented with a derive macro that would cover all members automatically, but it needed specialization despite my best efforts to hack 😆

A declarative macro gets a +1 from me.

[mejrs]

I once experimented with a derive macro

Could something like this work? With the __traverse__ impl above as an unsafe escape hatch?

#[pyclass]
struct GcIntegration {
    #[pyo3(gc)]
    self_ref: PyObject,
}

[davidhewitt]

Potentially; I think we'd probably be able to do something like have an unsafe trait PyGcTraversal, which we can then provide a blanket implementation for T: AsPyPointer as well as containers like Vec<T>.

It'll get a bit messy things like HashMap<K, V> where depending on K and V we'd need to figure out how the trait implementation varies depending on whether K and V implement PyGcTraversal. If I recall correctly that's where my previous hackery got stuck. At least with the approach of annotating all fields we could have #[pyo3(gc = "some_function")] to manually supply an escape hatch to traverse complex containers?