Merge pull request #835 from kngwyu/remove-new-no-ptr

Remove GILPool::new-no-pointer

Author: kngwyu

examples/rustapi_module/setup.py

@@ -93,11 +93,12 @@ def make_rust_extension(module_name):
     ],
     packages=["rustapi_module"],
     rust_extensions=[
-        make_rust_extension("rustapi_module.othermod"),
+        make_rust_extension("rustapi_module.buf_and_str"),
         make_rust_extension("rustapi_module.datetime"),
+        make_rust_extension("rustapi_module.objstore"),
+        make_rust_extension("rustapi_module.othermod"),
         make_rust_extension("rustapi_module.subclassing"),
         make_rust_extension("rustapi_module.test_dict"),
-        make_rust_extension("rustapi_module.buf_and_str"),
     ],
     install_requires=install_requires,
     tests_require=tests_require,

examples/rustapi_module/src/lib.rs

@@ -1,5 +1,6 @@
 pub mod buf_and_str;
 pub mod datetime;
 pub mod dict_iter;
+pub mod objstore;
 pub mod othermod;
 pub mod subclassing;

examples/rustapi_module/src/objstore.rs

@@ -0,0 +1,24 @@
+use pyo3::prelude::*;
+
+#[pyclass]
+#[derive(Default)]
+pub struct ObjStore {
+    obj: Vec<PyObject>,
+}
+
+#[pymethods]
+impl ObjStore {
+    #[new]
+    fn new() -> Self {
+        ObjStore::default()
+    }
+
+    fn push(&mut self, py: Python, obj: &PyAny) {
+        self.obj.push(obj.to_object(py));
+    }
+}
+
+#[pymodule]
+fn objstore(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
+    m.add_class::<ObjStore>()
+}

examples/rustapi_module/tests/test_objstore.py

@@ -0,0 +1,24 @@
+import gc
+import platform
+import sys
+
+import pytest
+
+from rustapi_module.objstore import ObjStore
+
+PYPY = platform.python_implementation() == "PyPy"
+
+
+@pytest.mark.skipif(PYPY, reason="PyPy does not have sys.getrefcount")
+def test_objstore_doesnot_leak_memory():
+    N = 10000
+    message = b'\\(-"-;) Praying that memory leak would not happen..'
+    before = sys.getrefcount(message)
+    store = ObjStore()
+    for _ in range(N):
+        store.push(message)
+    del store
+    gc.collect()
+    after = sys.getrefcount(message)
+
+    assert after - before == 0

src/gil.rs

@@ -107,7 +107,7 @@ impl Drop for GILGuard {
     fn drop(&mut self) {
         unsafe {
             let pool: &'static mut ReleasePool = &mut *POOL;
-            pool.drain(self.python(), self.owned, self.borrowed, true);
+            pool.drain(self.python(), self.owned, self.borrowed);
 
             ffi::PyGILState_Release(self.gstate);
         }
@@ -152,19 +152,15 @@ impl ReleasePool {
         vec.set_len(0);
     }
 
-    pub unsafe fn drain(&mut self, _py: Python, owned: usize, borrowed: usize, pointers: bool) {
+    pub unsafe fn drain(&mut self, _py: Python, owned: usize, borrowed: usize) {
         // Release owned objects(call decref)
         while owned < self.owned.len() {
             let last = self.owned.pop_back().unwrap();
             ffi::Py_DECREF(last.as_ptr());
         }
         // Release borrowed objects(don't call decref)
         self.borrowed.truncate(borrowed);
-
-        if pointers {
-            self.release_pointers();
-        }
-
+        self.release_pointers();
         self.obj.clear();
     }
 }
@@ -176,7 +172,6 @@ pub struct GILPool<'p> {
     py: Python<'p>,
     owned: usize,
     borrowed: usize,
-    pointers: bool,
     no_send: Unsendable,
 }
 
@@ -188,18 +183,6 @@ impl<'p> GILPool<'p> {
             py,
             owned: p.owned.len(),
             borrowed: p.borrowed.len(),
-            pointers: true,
-            no_send: Unsendable::default(),
-        }
-    }
-    #[inline]
-    pub fn new_no_pointers(py: Python) -> GILPool {
-        let p: &'static mut ReleasePool = unsafe { &mut *POOL };
-        GILPool {
-            py,
-            owned: p.owned.len(),
-            borrowed: p.borrowed.len(),
-            pointers: false,
             no_send: Unsendable::default(),
         }
     }
@@ -209,7 +192,7 @@ impl<'p> Drop for GILPool<'p> {
     fn drop(&mut self) {
         unsafe {
             let pool: &'static mut ReleasePool = &mut *POOL;
-            pool.drain(self.py, self.owned, self.borrowed, self.pointers);
+            pool.drain(self.py, self.owned, self.borrowed);
         }
     }
 }

src/pyclass.rs

@@ -112,7 +112,7 @@ where
         T: PyClassAlloc,
     {
         let py = Python::assume_gil_acquired();
-        let _pool = gil::GILPool::new_no_pointers(py);
+        let _pool = gil::GILPool::new(py);
         <T as PyClassAlloc>::dealloc(py, (obj as *mut T::Layout) as _)
     }
     type_object.tp_dealloc = Some(tp_dealloc_callback::<T>);

tests/test_gc.rs

@@ -2,8 +2,7 @@ use pyo3::class::PyGCProtocol;
 use pyo3::class::PyTraverseError;
 use pyo3::class::PyVisit;
 use pyo3::prelude::*;
-use pyo3::types::PyTuple;
-use pyo3::{ffi, py_run, AsPyPointer, PyCell, PyTryInto};
+use pyo3::{py_run, AsPyPointer, PyCell, PyTryInto};
 use std::cell::RefCell;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Arc;
@@ -81,52 +80,6 @@ fn data_is_dropped() {
     assert!(drop_called2.load(Ordering::Relaxed));
 }
 
-#[pyclass]
-struct ClassWithDrop {}
-
-impl Drop for ClassWithDrop {
-    fn drop(&mut self) {
-        unsafe {
-            let py = Python::assume_gil_acquired();
-
-            let _empty1: Py<PyTuple> = FromPy::from_py(PyTuple::empty(py), py);
-            let _empty2: PyObject = PyTuple::empty(py).into_py(py);
-            let _empty3: &PyAny = py.from_owned_ptr(ffi::PyTuple_New(0));
-        }
-    }
-}
-
-// Test behavior of pythonrun::register_pointers + type_object::dealloc
-#[test]
-fn create_pointers_in_drop() {
-    let _gil = Python::acquire_gil();
-
-    let ptr;
-    let cnt;
-    {
-        let gil = Python::acquire_gil();
-        let py = gil.python();
-        let empty: PyObject = PyTuple::empty(py).into_py(py);
-        ptr = empty.as_ptr();
-        // substract 2, because `PyTuple::empty(py).into_py(py)` increases the refcnt by 2
-        cnt = empty.get_refcnt() - 2;
-        let inst = Py::new(py, ClassWithDrop {}).unwrap();
-        drop(inst);
-    }
-
-    // empty1 and empty2 are still alive (stored in pointers list)
-    {
-        let _gil = Python::acquire_gil();
-        assert_eq!(cnt + 2, unsafe { ffi::Py_REFCNT(ptr) });
-    }
-
-    // empty1 and empty2 should be released
-    {
-        let _gil = Python::acquire_gil();
-        assert_eq!(cnt, unsafe { ffi::Py_REFCNT(ptr) });
-    }
-}
-
 #[allow(dead_code)]
 #[pyclass]
 struct GCIntegration {