Allow non-mutable reference in PyIterProtocol::__iter__

[althonos]

This is both a bug report and an improvement request concerning the PyIterProtocol trait.

🐛 Bug Reports

🌍 Environment

• Your operating system and version: Archlinux, Linux 5.3.8
• Your python version: Python 3.8
• How did you install python (e.g. apt or pyenv)? Did you use a virtualenv?: pacman
• Your rust version (rustc --version): 1.43.0-nightly
• Are you using the latest pyo3 version? Have you tried using latest master (replace version = "0.x.y" with git = "https://github.com/PyO3/pyo3")? using master

💥 Reproducing

(complete code in althonos/nanoset.py repo on the experimental branch)

I have a Rust struct wrapping a single PyObject, which I expose both in the PyIterProtocol to call the wrapped object __iter__, and in the PyGCProtocol so that it's traversed and garbage collected as expected:

#[pyclass(gc)]
pub struct MyWrapper {
    inner: PyObject,
}

#[pyproto]
impl PyIterProtocol for MyWrapper {
    fn __iter__(slf: PyMutRef<Self>) -> PyResult<PyObject> {
        let gil = Python::acquire_gil();
        let py = gil.python();
        slf.inner.call_method0(py, "__iter__")
    }
}

#[pyproto]
impl PyGCProtocol for MyWrapper {
    fn __traverse__(&'p self, visit: PyVisit) -> Result<(), PyTraverseError> {
        visit.call(self.inner)
    }

    fn __clear__(&'p mut self) {
        let gil = Python::acquire_gil();
        gil.python().release(self.inner)
    }
}

However, when running this within Python, I get some panics whenever the GC tries to traverse my object and i'm requesting iteration of the object at about the same time. That can happen when iterating on the object from within a function it was passed as an argument of. I get the following backtrace:

thread '<unnamed>' panicked at 'Already mutably borrowed: PyBorrowError', /home/althonos/.local/share/cargo/registry/src/github.com-1ecc6299db9ec823/pyo3-0.9.1/src/pycell.rs:183:9
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: std::panicking::rust_panic_with_hook
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::result::unwrap_failed
   9: <T as pyo3::class::gc::PyGCTraverseProtocolImpl>::tp_traverse::tp_traverse
  10: <unknown>
  11: <unknown>
  12: _PyObject_GC_New
  13: <unknown>
  14: PyObject_Call
  15: <T as pyo3::conversion::ToBorrowedObject>::with_borrowed_ptr
  16: pyo3::object::PyObject::call_method0
  17: <my_wrapper::MyWrapper as pyo3::class::iter::PyIterProtocol>::__iter__
  18: <T as pyo3::class::iter::PyIterIterProtocolImpl>::tp_iter::wrap
  19: PyObject_GetIter
  20: <unknown>
  21: <unknown>

which suggests that pyo3 tried to borrow mutably MyWrapper twice.

However, I don't actually need a mutable reference in __iter__, so i tried tinkering with the pyo3 code, and I got my code to work without panics after chaning the PyIteratorProtocol::__iter__ signature only to take a PyRef.

This leads to my following question: would it be possible to make __iter__ accept either PyRef or PyMutRef, like other methods support several arbitrary types?

[davidhewitt]

Completely agree with you.

I think that __iter__ being able to work with PyRef or even &self as well as PyRefMut and &mut self would be a desirable long-term goal.

I think at the moment it might be complicated to add this flexibility, and this code is due to be rewritten to remove specialization anyway, so I'm not sure how quickly we can make a change to support this...

[althonos]

No emergency on my end, I can just disable GC integration for the time being. On a side note, I saw some more of these panics caused by double borrowing within the GC, i'll try to investigate in case this is a more serious issue with the garbage collector.

[davidhewitt]

Cool, thanks. To guide your investigation there's probably a point in the GC code where we're currently call borrow_mut() but probably should be using try_borrow_mut()

(Unless the GC code itself has some kind of bug where it's dependent on double mutable borrow)

[althonos]

@davidhewitt : that's exactly it! I'll open two separate PRs, one for the GC and a draft one for the current issue, since I have a working WIP implementation that could be refined further IMO.

[davidhewitt]

Fixed after #855 and #856. Many thanks @althonos