D6631: rust-cpython: add macro for sharing references

yuja (Yuya Nishihara) phabricator at mercurial-scm.org
Sat Aug 17 04:43:24 EDT 2019


yuja added a comment.


  > +impl PySharedState {
  > +    pub fn borrow_mut<'a, T>(
  > +        &'a self,
  > +        py: Python<'a>,
  > +        pyrefmut: RefMut<'a, T>,
  > +    ) -> PyResult<PyRefMut<'a, T>> {
  > +        if self.mutably_borrowed.get() {
  > +            return Err(AlreadyBorrowed::new(
  > +                py,
  > +                "Cannot borrow mutably while there exists another \
  > +                 mutable reference in a Python object",
  > +            ));
  > +        }
  > +        match self.leak_count.get() {
  > +            0 => {
  > +                self.mutably_borrowed.replace(true);
  > +                Ok(PyRefMut::new(py, pyrefmut, self))
  > +            }
  > +            // TODO
  > +            // For now, this works differently than Python references
  > +            // in the case of iterators.
  > +            // Python does not complain when the data an iterator
  > +            // points to is modified if the iterator is never used
  > +            // afterwards.
  > +            // Here, we are stricter than this by refusing to give a
  > +            // mutable reference if it is already borrowed.
  > +            // While the additional safety might be argued for, it
  > +            // breaks valid programming patterns in Python and we need
  > +            // to fix this issue down the line.
  > +            _ => Err(AlreadyBorrowed::new(
  > +                py,
  > +                "Cannot borrow mutably while there are \
  > +                 immutable references in Python objects",
  > +            )),
  > +        }
  > +    }
  > +
  > +    /// Return a reference to the wrapped data with an artificial static
  > +    /// lifetime.
  > +    /// We need to be protected by the GIL for thread-safety.
  > +    pub fn leak_immutable<T>(
  > +        &self,
  > +        py: Python,
  > +        data: &RefCell<T>,
  > +    ) -> PyResult<&'static T> {
  > +        if self.mutably_borrowed.get() {
  > +            return Err(AlreadyBorrowed::new(
  > +                py,
  > +                "Cannot borrow immutably while there is a \
  > +                 mutable reference in Python objects",
  > +            ));
  > +        }
  > +        let ptr = data.as_ptr();
  > +        self.leak_count.replace(self.leak_count.get() + 1);
  > +        unsafe { Ok(&*ptr) }
  > +    }
  
  This leak_immutable() looks potentially unsafe since it can extend the
  lifetime of an arbitrary object, data.
  
  > +macro_rules! py_shared_ref {
  > +    (
  > +        $name: ident,
  > +        $inner_struct: ident,
  > +        $data_member: ident,
  > +        $leaked: ident,
  > +    ) => {
  > +        impl $name {
  > +            fn borrow_mut<'a>(
  > +                &'a self,
  > +                py: Python<'a>,
  > +            ) -> PyResult<crate::ref_sharing::PyRefMut<'a, $inner_struct>>
  > +            {
  > +                self.py_shared_state(py)
  > +                    .borrow_mut(py, self.$data_member(py).borrow_mut())
  > +            }
  > +
  > +            fn leak_immutable<'a>(
  > +                &'a self,
  > +                py: Python<'a>,
  > +            ) -> PyResult<&'static $inner_struct> {
  > +                self.py_shared_state(py)
  > +                    .leak_immutable(py, self.$data_member(py))
  > +            }
  > +        }
  
  Is it okay to directly borrow/borrow_mut() the inner object?
  
  For example, we can still do `self.inner(py).borrow_mut()` (in place of
  `self.borrow_mut(py)`) while the inner object is leaked, which I think is
  unsafe.
  
  If I understand the py-shared logic correctly, PySharedState needs to own
  the inner object to guarantee that any public operations are safe.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6631/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6631

To: Alphare, #hg-reviewers, kevincox
Cc: yuja, gracinet, durin42, kevincox, mercurial-devel


More information about the Mercurial-devel mailing list