rust-cpython: require GIL to borrow immutable reference from PySharedRefCell
authorYuya Nishihara <yuya@tcha.org>
Sat, 21 Sep 2019 17:15:50 +0900
changeset 43286 f8c114f20d2d
parent 43285 ffc1fbd7d1f5
child 43287 0df8312463ae
rust-cpython: require GIL to borrow immutable reference from PySharedRefCell Since the inner value may be leaked, we probably need GIL to guarantee that there's no data race. inner(py).borrow() is replaced with inner_shared(py).borrow(), which basically means any PySharedRefCell data should be accessed through PySharedRef wrapper.
rust/hg-cpython/src/dirstate/dirs_multiset.rs
rust/hg-cpython/src/dirstate/dirstate_map.rs
rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/dirstate/dirs_multiset.rs	Sun Sep 15 22:19:10 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/dirs_multiset.rs	Sat Sep 21 17:15:50 2019 +0900
@@ -100,7 +100,7 @@
     }
 
     def __contains__(&self, item: PyObject) -> PyResult<bool> {
-        Ok(self.inner(py).borrow().contains(HgPath::new(
+        Ok(self.inner_shared(py).borrow().contains(HgPath::new(
             item.extract::<PyBytes>(py)?.data(py).as_ref(),
         )))
     }
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs	Sun Sep 15 22:19:10 2019 +0900
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs	Sat Sep 21 17:15:50 2019 +0900
@@ -63,7 +63,7 @@
         default: Option<PyObject> = None
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
-        match self.inner(py).borrow().get(HgPath::new(key.data(py))) {
+        match self.inner_shared(py).borrow().get(HgPath::new(key.data(py))) {
             Some(entry) => {
                 Ok(Some(make_dirstate_tuple(py, entry)?))
             },
@@ -170,7 +170,7 @@
     // TODO share the reference
     def nonnormalentries(&self) -> PyResult<PyObject> {
         let (non_normal, other_parent) =
-            self.inner(py).borrow().non_normal_other_parent_entries();
+            self.inner_shared(py).borrow().non_normal_other_parent_entries();
 
         let locals = PyDict::new(py);
         locals.set_item(
@@ -281,18 +281,18 @@
     }
 
     def __len__(&self) -> PyResult<usize> {
-        Ok(self.inner(py).borrow().len())
+        Ok(self.inner_shared(py).borrow().len())
     }
 
     def __contains__(&self, key: PyObject) -> PyResult<bool> {
         let key = key.extract::<PyBytes>(py)?;
-        Ok(self.inner(py).borrow().contains_key(HgPath::new(key.data(py))))
+        Ok(self.inner_shared(py).borrow().contains_key(HgPath::new(key.data(py))))
     }
 
     def __getitem__(&self, key: PyObject) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
         let key = HgPath::new(key.data(py));
-        match self.inner(py).borrow().get(key) {
+        match self.inner_shared(py).borrow().get(key) {
             Some(entry) => {
                 Ok(make_dirstate_tuple(py, entry)?)
             },
@@ -333,7 +333,7 @@
         Dirs::from_inner(
             py,
             DirsMultiset::from_dirstate(
-                &self.inner(py).borrow(),
+                &self.inner_shared(py).borrow(),
                 Some(EntryState::Removed),
             ),
         )
@@ -344,7 +344,7 @@
         Dirs::from_inner(
             py,
             DirsMultiset::from_dirstate(
-                &self.inner(py).borrow(),
+                &self.inner_shared(py).borrow(),
                 None,
             ),
         )
@@ -353,7 +353,7 @@
     // TODO all copymap* methods, see docstring above
     def copymapcopy(&self) -> PyResult<PyDict> {
         let dict = PyDict::new(py);
-        for (key, value) in self.inner(py).borrow().copy_map.iter() {
+        for (key, value) in self.inner_shared(py).borrow().copy_map.iter() {
             dict.set_item(
                 py,
                 PyBytes::new(py, key.as_ref()),
@@ -365,7 +365,7 @@
 
     def copymapgetitem(&self, key: PyObject) -> PyResult<PyBytes> {
         let key = key.extract::<PyBytes>(py)?;
-        match self.inner(py).borrow().copy_map.get(HgPath::new(key.data(py))) {
+        match self.inner_shared(py).borrow().copy_map.get(HgPath::new(key.data(py))) {
             Some(copy) => Ok(PyBytes::new(py, copy.as_ref())),
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
@@ -378,12 +378,12 @@
     }
 
     def copymaplen(&self) -> PyResult<usize> {
-        Ok(self.inner(py).borrow().copy_map.len())
+        Ok(self.inner_shared(py).borrow().copy_map.len())
     }
     def copymapcontains(&self, key: PyObject) -> PyResult<bool> {
         let key = key.extract::<PyBytes>(py)?;
         Ok(self
-            .inner(py)
+            .inner_shared(py)
             .borrow()
             .copy_map
             .contains_key(HgPath::new(key.data(py))))
@@ -395,7 +395,7 @@
     ) -> PyResult<Option<PyObject>> {
         let key = key.extract::<PyBytes>(py)?;
         match self
-            .inner(py)
+            .inner_shared(py)
             .borrow()
             .copy_map
             .get(HgPath::new(key.data(py)))
--- a/rust/hg-cpython/src/ref_sharing.rs	Sun Sep 15 22:19:10 2019 +0900
+++ b/rust/hg-cpython/src/ref_sharing.rs	Sat Sep 21 17:15:50 2019 +0900
@@ -136,7 +136,7 @@
         }
     }
 
-    pub fn borrow(&self) -> Ref<T> {
+    pub fn borrow<'a>(&'a self, _py: Python<'a>) -> Ref<'a, T> {
         // py_shared_state isn't involved since
         // - inner.borrow() would fail if self is mutably borrowed,
         // - and inner.borrow_mut() would fail while self is borrowed.
@@ -180,7 +180,7 @@
     }
 
     pub fn borrow(&self) -> Ref<'a, T> {
-        self.data.borrow()
+        self.data.borrow(self.py)
     }
 
     pub fn borrow_mut(&self) -> PyResult<PyRefMut<'a, T>> {