rust-cpython: make PyLeakedRef operations relatively safe
This patch encapsulates the access to the leaked reference to make most
leaked-ref operations safe. The only exception is leaked_ref.map(). I
couldn't figure out how to allow arbitrary map operation safely over an
unsafe static reference. See the docstring and inline comment for details.
Now leak_immutable() can be safely implemented as the PyLeakedRef owns
its inner data.
--- a/rust/hg-cpython/src/dirstate/copymap.rs Sun Sep 15 22:06:19 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/copymap.rs Sun Sep 15 22:19:10 2019 +0900
@@ -13,7 +13,6 @@
use crate::dirstate::dirstate_map::DirstateMap;
use crate::ref_sharing::PyLeakedRef;
-use hg::DirstateMap as RustDirstateMap;
use hg::{utils::hg_path::HgPathBuf, CopyMapIter};
py_class!(pub class CopyMap |py| {
@@ -105,16 +104,14 @@
py_shared_iterator!(
CopyMapKeysIterator,
- PyLeakedRef<&'static RustDirstateMap>,
- CopyMapIter<'static>,
+ PyLeakedRef<CopyMapIter<'static>>,
CopyMap::translate_key,
Option<PyBytes>
);
py_shared_iterator!(
CopyMapItemsIterator,
- PyLeakedRef<&'static RustDirstateMap>,
- CopyMapIter<'static>,
+ PyLeakedRef<CopyMapIter<'static>>,
CopyMap::translate_key_value,
Option<(PyBytes, PyBytes)>
);
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs Sun Sep 15 22:06:19 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs Sun Sep 15 22:19:10 2019 +0900
@@ -92,13 +92,10 @@
})
}
def __iter__(&self) -> PyResult<DirsMultisetKeysIterator> {
- let mut leak_handle =
- unsafe { self.inner_shared(py).leak_immutable()? };
- let leaked_ref = leak_handle.data.take().unwrap();
+ let leaked_ref = self.inner_shared(py).leak_immutable()?;
DirsMultisetKeysIterator::from_inner(
py,
- leak_handle,
- leaked_ref.iter(),
+ unsafe { leaked_ref.map(py, |o| o.iter()) },
)
}
@@ -126,8 +123,7 @@
py_shared_iterator!(
DirsMultisetKeysIterator,
- PyLeakedRef<&'static DirsMultiset>,
- DirsMultisetIter<'static>,
+ PyLeakedRef<DirsMultisetIter<'static>>,
Dirs::translate_key,
Option<PyBytes>
);
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs Sun Sep 15 22:06:19 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs Sun Sep 15 22:19:10 2019 +0900
@@ -304,35 +304,26 @@
}
def keys(&self) -> PyResult<DirstateMapKeysIterator> {
- let mut leak_handle =
- unsafe { self.inner_shared(py).leak_immutable()? };
- let leaked_ref = leak_handle.data.take().unwrap();
+ let leaked_ref = self.inner_shared(py).leak_immutable()?;
DirstateMapKeysIterator::from_inner(
py,
- leak_handle,
- leaked_ref.iter(),
+ unsafe { leaked_ref.map(py, |o| o.iter()) },
)
}
def items(&self) -> PyResult<DirstateMapItemsIterator> {
- let mut leak_handle =
- unsafe { self.inner_shared(py).leak_immutable()? };
- let leaked_ref = leak_handle.data.take().unwrap();
+ let leaked_ref = self.inner_shared(py).leak_immutable()?;
DirstateMapItemsIterator::from_inner(
py,
- leak_handle,
- leaked_ref.iter(),
+ unsafe { leaked_ref.map(py, |o| o.iter()) },
)
}
def __iter__(&self) -> PyResult<DirstateMapKeysIterator> {
- let mut leak_handle =
- unsafe { self.inner_shared(py).leak_immutable()? };
- let leaked_ref = leak_handle.data.take().unwrap();
+ let leaked_ref = self.inner_shared(py).leak_immutable()?;
DirstateMapKeysIterator::from_inner(
py,
- leak_handle,
- leaked_ref.iter(),
+ unsafe { leaked_ref.map(py, |o| o.iter()) },
)
}
@@ -446,24 +437,18 @@
}
def copymapiter(&self) -> PyResult<CopyMapKeysIterator> {
- let mut leak_handle =
- unsafe { self.inner_shared(py).leak_immutable()? };
- let leaked_ref = leak_handle.data.take().unwrap();
+ let leaked_ref = self.inner_shared(py).leak_immutable()?;
CopyMapKeysIterator::from_inner(
py,
- leak_handle,
- leaked_ref.copy_map.iter(),
+ unsafe { leaked_ref.map(py, |o| o.copy_map.iter()) },
)
}
def copymapitemsiter(&self) -> PyResult<CopyMapItemsIterator> {
- let mut leak_handle =
- unsafe { self.inner_shared(py).leak_immutable()? };
- let leaked_ref = leak_handle.data.take().unwrap();
+ let leaked_ref = self.inner_shared(py).leak_immutable()?;
CopyMapItemsIterator::from_inner(
py,
- leak_handle,
- leaked_ref.copy_map.iter(),
+ unsafe { leaked_ref.map(py, |o| o.copy_map.iter()) },
)
}
@@ -498,16 +483,14 @@
py_shared_iterator!(
DirstateMapKeysIterator,
- PyLeakedRef<&'static RustDirstateMap>,
- StateMapIter<'static>,
+ PyLeakedRef<StateMapIter<'static>>,
DirstateMap::translate_key,
Option<PyBytes>
);
py_shared_iterator!(
DirstateMapItemsIterator,
- PyLeakedRef<&'static RustDirstateMap>,
- StateMapIter<'static>,
+ PyLeakedRef<StateMapIter<'static>>,
DirstateMap::translate_key_value,
Option<(PyBytes, PyObject)>
);
--- a/rust/hg-cpython/src/ref_sharing.rs Sun Sep 15 22:06:19 2019 +0900
+++ b/rust/hg-cpython/src/ref_sharing.rs Sun Sep 15 22:19:10 2019 +0900
@@ -187,24 +187,19 @@
self.data.borrow_mut(self.py)
}
- /// Returns a leaked reference temporarily held by its management object.
- ///
- /// # Safety
- ///
- /// It's up to you to make sure that the management object lives
- /// longer than the leaked reference. Otherwise, you'll get a
- /// dangling reference.
- pub unsafe fn leak_immutable(&self) -> PyResult<PyLeakedRef<&'static T>> {
- let (static_ref, static_state_ref) = self
- .data
- .py_shared_state
- .leak_immutable(self.py, self.data)?;
- Ok(PyLeakedRef::new(
- self.py,
- self.owner,
- static_ref,
- static_state_ref,
- ))
+ /// Returns a leaked reference.
+ pub fn leak_immutable(&self) -> PyResult<PyLeakedRef<&'static T>> {
+ let state = &self.data.py_shared_state;
+ unsafe {
+ let (static_ref, static_state_ref) =
+ state.leak_immutable(self.py, self.data)?;
+ Ok(PyLeakedRef::new(
+ self.py,
+ self.owner,
+ static_ref,
+ static_state_ref,
+ ))
+ }
}
}
@@ -316,15 +311,15 @@
}
/// Manage immutable references to `PyObject` leaked into Python iterators.
-///
-/// In truth, this does not represent leaked references themselves;
-/// it is instead useful alongside them to manage them.
pub struct PyLeakedRef<T> {
- _inner: PyObject,
- pub data: Option<T>, // TODO: remove pub
+ inner: PyObject,
+ data: Option<T>,
py_shared_state: &'static PySharedState,
}
+// DO NOT implement Deref for PyLeakedRef<T>! Dereferencing PyLeakedRef
+// without taking Python GIL wouldn't be safe.
+
impl<T> PyLeakedRef<T> {
/// # Safety
///
@@ -338,11 +333,52 @@
py_shared_state: &'static PySharedState,
) -> Self {
Self {
- _inner: inner.clone_ref(py),
+ inner: inner.clone_ref(py),
data: Some(data),
py_shared_state,
}
}
+
+ /// Returns an immutable reference to the inner value.
+ pub fn get_ref<'a>(&'a self, _py: Python<'a>) -> &'a T {
+ self.data.as_ref().unwrap()
+ }
+
+ /// Returns a mutable reference to the inner value.
+ ///
+ /// Typically `T` is an iterator. If `T` is an immutable reference,
+ /// `get_mut()` is useless since the inner value can't be mutated.
+ pub fn get_mut<'a>(&'a mut self, _py: Python<'a>) -> &'a mut T {
+ self.data.as_mut().unwrap()
+ }
+
+ /// Converts the inner value by the given function.
+ ///
+ /// Typically `T` is a static reference to a container, and `U` is an
+ /// iterator of that container.
+ ///
+ /// # Safety
+ ///
+ /// The lifetime of the object passed in to the function `f` is cheated.
+ /// It's typically a static reference, but is valid only while the
+ /// corresponding `PyLeakedRef` is alive. Do not copy it out of the
+ /// function call.
+ pub unsafe fn map<U>(
+ mut self,
+ py: Python,
+ f: impl FnOnce(T) -> U,
+ ) -> PyLeakedRef<U> {
+ // f() could make the self.data outlive. That's why map() is unsafe.
+ // In order to make this function safe, maybe we'll need a way to
+ // temporarily restrict the lifetime of self.data and translate the
+ // returned object back to Something<'static>.
+ let new_data = f(self.data.take().unwrap());
+ PyLeakedRef {
+ inner: self.inner.clone_ref(py),
+ data: Some(new_data),
+ py_shared_state: self.py_shared_state,
+ }
+ }
}
impl<T> Drop for PyLeakedRef<T> {
@@ -352,6 +388,9 @@
// sure that the state is only accessed by this thread.
let gil = Python::acquire_gil();
let py = gil.python();
+ if self.data.is_none() {
+ return; // moved to another PyLeakedRef
+ }
unsafe {
self.py_shared_state.decrease_leak_count(py, false);
}
@@ -383,13 +422,10 @@
/// data inner: PySharedRefCell<MyStruct>;
///
/// def __iter__(&self) -> PyResult<MyTypeItemsIterator> {
-/// let mut leak_handle =
-/// unsafe { self.inner_shared(py).leak_immutable()? };
-/// let leaked_ref = leak_handle.data.take().unwrap();
+/// let leaked_ref = self.inner_shared(py).leak_immutable()?;
/// MyTypeItemsIterator::from_inner(
/// py,
-/// leak_handle,
-/// leaked_ref.iter(),
+/// unsafe { leaked_ref.map(py, |o| o.iter()) },
/// )
/// }
/// });
@@ -411,8 +447,7 @@
///
/// py_shared_iterator!(
/// MyTypeItemsIterator,
-/// PyLeakedRef<&'static MyStruct>,
-/// HashMap<'static, Vec<u8>, Vec<u8>>,
+/// PyLeakedRef<HashMap<'static, Vec<u8>, Vec<u8>>>,
/// MyType::translate_key_value,
/// Option<(PyBytes, PyBytes)>
/// );
@@ -421,18 +456,16 @@
(
$name: ident,
$leaked: ty,
- $iterator_type: ty,
$success_func: expr,
$success_type: ty
) => {
py_class!(pub class $name |py| {
data inner: RefCell<Option<$leaked>>;
- data it: RefCell<$iterator_type>;
def __next__(&self) -> PyResult<$success_type> {
let mut inner_opt = self.inner(py).borrow_mut();
- if inner_opt.is_some() {
- match self.it(py).borrow_mut().next() {
+ if let Some(leaked) = inner_opt.as_mut() {
+ match leaked.get_mut(py).next() {
None => {
// replace Some(inner) by None, drop $leaked
inner_opt.take();
@@ -456,12 +489,10 @@
pub fn from_inner(
py: Python,
leaked: $leaked,
- it: $iterator_type
) -> PyResult<Self> {
Self::create_instance(
py,
RefCell::new(Some(leaked)),
- RefCell::new(it)
)
}
}