From 34099b33f85e8fd826f09b2386e14fbdcc833ef4 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Tue, 13 Nov 2018 00:24:59 +0900 Subject: [PATCH 1/4] Fix PyDictIterator's segfault(for #159) --- examples/rustapi_module/setup.py | 5 +++ examples/rustapi_module/src/dict_iter.rs | 42 +++++++++++++++++++ examples/rustapi_module/src/lib.rs | 1 + .../rustapi_module/tests/test_dict_iter.py | 12 ++++++ src/types/dict.rs | 16 ++++--- 5 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 examples/rustapi_module/src/dict_iter.rs create mode 100644 examples/rustapi_module/tests/test_dict_iter.py diff --git a/examples/rustapi_module/setup.py b/examples/rustapi_module/setup.py index 7be4bdea..1b9ed7f6 100644 --- a/examples/rustapi_module/setup.py +++ b/examples/rustapi_module/setup.py @@ -60,6 +60,11 @@ setup( "Cargo.toml", rustc_flags=get_py_version_cfgs(), ), + RustExtension( + "rustapi_module._test_dict", + "Cargo.toml", + rustc_flags=get_py_version_cfgs(), + ), ], install_requires=install_requires, tests_require=tests_require, diff --git a/examples/rustapi_module/src/dict_iter.rs b/examples/rustapi_module/src/dict_iter.rs new file mode 100644 index 00000000..7394b156 --- /dev/null +++ b/examples/rustapi_module/src/dict_iter.rs @@ -0,0 +1,42 @@ +use pyo3::prelude::*; + +use pyo3::exceptions as exc; +use pyo3::types::PyDict; + +#[pymodinit(_test_dict)] +fn test_dict(_py: Python, m: &PyModule) -> PyResult<()> { + m.add_class::()?; + Ok(()) +} + +#[pyclass] +pub struct DictSize { + expected: u32, +} + +#[pymethods] +impl DictSize { + #[new] + fn __new__(obj: &PyRawObject, expected: u32) -> PyResult<()> { + obj.init(|_t| DictSize { expected }) + } + + fn iter_dict(&mut self, _py: Python, dict: &PyDict) -> PyResult { + let mut seen = 0u32; + for (sym, values) in dict.iter() { + seen += 1; + println!( + "{:4}/{:4} iterations:{}=>{}", + seen, self.expected, sym, values + ); + } + + match seen == self.expected { + true => Ok(seen), + _ => Err(PyErr::new::(format!( + "Expected {} iterations - performed {}", + self.expected, seen + ))), + } + } +} diff --git a/examples/rustapi_module/src/lib.rs b/examples/rustapi_module/src/lib.rs index f0b66eb6..36f8a7b3 100644 --- a/examples/rustapi_module/src/lib.rs +++ b/examples/rustapi_module/src/lib.rs @@ -4,5 +4,6 @@ extern crate pyo3; pub mod datetime; +pub mod dict_iter; pub mod othermod; pub mod subclassing; diff --git a/examples/rustapi_module/tests/test_dict_iter.py b/examples/rustapi_module/tests/test_dict_iter.py new file mode 100644 index 00000000..b5878867 --- /dev/null +++ b/examples/rustapi_module/tests/test_dict_iter.py @@ -0,0 +1,12 @@ +from rustapi_module._test_dict import DictSize +import pytest + +@pytest.mark.parametrize( + "size", + [64, 128, 256], +) +def test_size(size): + d = {} + for i in range(0,size): + d[i] = str(i) + assert DictSize(len(d)).iter_dict(d) == size diff --git a/src/types/dict.rs b/src/types/dict.rs index 00da0bdf..22d4da23 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -8,7 +8,7 @@ use crate::object::PyObject; use crate::python::{IntoPyPointer, Python, ToPyPointer}; use crate::types::{PyList, PyObjectRef}; use std; -use std::{cmp, collections, hash, mem}; +use std::{cmp, collections, hash, marker::PhantomData, mem}; /// Represents a Python `dict`. #[repr(transparent)] @@ -148,13 +148,19 @@ impl PyDict { /// Note that it's unsafe to use when the dictionary might be changed /// by other python code. pub fn iter(&self) -> PyDictIterator { - PyDictIterator { dict: self, pos: 0 } + let py = self.py(); + PyDictIterator { + dict: self.to_object(py), + pos: 0, + __marker: PhantomData, + } } } -pub struct PyDictIterator<'a> { - dict: &'a PyDict, +pub struct PyDictIterator<'dict> { + dict: PyObject, pos: isize, + __marker: PhantomData<&'dict ()>, } impl<'a> Iterator for PyDictIterator<'a> { @@ -166,7 +172,7 @@ impl<'a> Iterator for PyDictIterator<'a> { let mut key: *mut ffi::PyObject = mem::uninitialized(); let mut value: *mut ffi::PyObject = mem::uninitialized(); if ffi::PyDict_Next(self.dict.as_ptr(), &mut self.pos, &mut key, &mut value) != 0 { - let py = self.dict.py(); + let py = Python::assume_gil_acquired(); Some((py.from_borrowed_ptr(key), py.from_borrowed_ptr(value))) } else { None From 3b01b8f6ba726733803362f674c8e25471f2f400 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Tue, 13 Nov 2018 00:53:06 +0900 Subject: [PATCH 2/4] Review fixes --- examples/rustapi_module/setup.py | 2 +- examples/rustapi_module/src/dict_iter.rs | 15 +++++++------- .../rustapi_module/tests/test_dict_iter.py | 2 +- src/types/dict.rs | 20 ++++++++++++------- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/examples/rustapi_module/setup.py b/examples/rustapi_module/setup.py index 1b9ed7f6..99baa66c 100644 --- a/examples/rustapi_module/setup.py +++ b/examples/rustapi_module/setup.py @@ -61,7 +61,7 @@ setup( rustc_flags=get_py_version_cfgs(), ), RustExtension( - "rustapi_module._test_dict", + "rustapi_module.test_dict", "Cargo.toml", rustc_flags=get_py_version_cfgs(), ), diff --git a/examples/rustapi_module/src/dict_iter.rs b/examples/rustapi_module/src/dict_iter.rs index 7394b156..a5cfaad6 100644 --- a/examples/rustapi_module/src/dict_iter.rs +++ b/examples/rustapi_module/src/dict_iter.rs @@ -1,9 +1,9 @@ use pyo3::prelude::*; -use pyo3::exceptions as exc; +use pyo3::exceptions::RuntimeError; use pyo3::types::PyDict; -#[pymodinit(_test_dict)] +#[pymodinit(test_dict)] fn test_dict(_py: Python, m: &PyModule) -> PyResult<()> { m.add_class::()?; Ok(()) @@ -18,7 +18,7 @@ pub struct DictSize { impl DictSize { #[new] fn __new__(obj: &PyRawObject, expected: u32) -> PyResult<()> { - obj.init(|_t| DictSize { expected }) + obj.init(|| DictSize { expected }) } fn iter_dict(&mut self, _py: Python, dict: &PyDict) -> PyResult { @@ -31,12 +31,13 @@ impl DictSize { ); } - match seen == self.expected { - true => Ok(seen), - _ => Err(PyErr::new::(format!( + if seen == self.expected { + Ok(seen) + } else { + Err(PyErr::new::(format!( "Expected {} iterations - performed {}", self.expected, seen - ))), + ))) } } } diff --git a/examples/rustapi_module/tests/test_dict_iter.py b/examples/rustapi_module/tests/test_dict_iter.py index b5878867..c0af56a0 100644 --- a/examples/rustapi_module/tests/test_dict_iter.py +++ b/examples/rustapi_module/tests/test_dict_iter.py @@ -7,6 +7,6 @@ import pytest ) def test_size(size): d = {} - for i in range(0,size): + for i in range(size): d[i] = str(i) assert DictSize(len(d)).iter_dict(d) == size diff --git a/src/types/dict.rs b/src/types/dict.rs index 22d4da23..4ecd1fe0 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -8,7 +8,7 @@ use crate::object::PyObject; use crate::python::{IntoPyPointer, Python, ToPyPointer}; use crate::types::{PyList, PyObjectRef}; use std; -use std::{cmp, collections, hash, marker::PhantomData, mem}; +use std::{cmp, collections, hash, mem, ops::Drop}; /// Represents a Python `dict`. #[repr(transparent)] @@ -152,19 +152,19 @@ impl PyDict { PyDictIterator { dict: self.to_object(py), pos: 0, - __marker: PhantomData, + py, } } } -pub struct PyDictIterator<'dict> { +pub struct PyDictIterator<'py> { dict: PyObject, pos: isize, - __marker: PhantomData<&'dict ()>, + py: Python<'py>, } -impl<'a> Iterator for PyDictIterator<'a> { - type Item = (&'a PyObjectRef, &'a PyObjectRef); +impl<'py> Iterator for PyDictIterator<'py> { + type Item = (&'py PyObjectRef, &'py PyObjectRef); #[inline] fn next(&mut self) -> Option { @@ -172,7 +172,7 @@ impl<'a> Iterator for PyDictIterator<'a> { let mut key: *mut ffi::PyObject = mem::uninitialized(); let mut value: *mut ffi::PyObject = mem::uninitialized(); if ffi::PyDict_Next(self.dict.as_ptr(), &mut self.pos, &mut key, &mut value) != 0 { - let py = Python::assume_gil_acquired(); + let py = self.py; Some((py.from_borrowed_ptr(key), py.from_borrowed_ptr(value))) } else { None @@ -181,6 +181,12 @@ impl<'a> Iterator for PyDictIterator<'a> { } } +impl<'py> Drop for PyDictIterator<'py> { + fn drop(&mut self) { + unsafe { ffi::Py_DECREF(self.dict.as_ptr()) } + } +} + impl<'a> std::iter::IntoIterator for &'a PyDict { type Item = (&'a PyObjectRef, &'a PyObjectRef); type IntoIter = PyDictIterator<'a>; From faa5efc5a1b93549d0a1d367d89b29ad57003837 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Tue, 13 Nov 2018 01:00:15 +0900 Subject: [PATCH 3/4] Remove unnecessary Drop for PyDictIterator --- src/types/dict.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 4ecd1fe0..9434c7b3 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -181,12 +181,6 @@ impl<'py> Iterator for PyDictIterator<'py> { } } -impl<'py> Drop for PyDictIterator<'py> { - fn drop(&mut self) { - unsafe { ffi::Py_DECREF(self.dict.as_ptr()) } - } -} - impl<'a> std::iter::IntoIterator for &'a PyDict { type Item = (&'a PyObjectRef, &'a PyObjectRef); type IntoIter = PyDictIterator<'a>; From 1081ba9447b54759024609fbcafe759a2b6983e7 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Tue, 13 Nov 2018 11:50:13 +0900 Subject: [PATCH 4/4] Fix import module name in test_dict_iter --- examples/rustapi_module/tests/test_dict_iter.py | 3 ++- src/types/dict.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/examples/rustapi_module/tests/test_dict_iter.py b/examples/rustapi_module/tests/test_dict_iter.py index c0af56a0..98acc214 100644 --- a/examples/rustapi_module/tests/test_dict_iter.py +++ b/examples/rustapi_module/tests/test_dict_iter.py @@ -1,6 +1,7 @@ -from rustapi_module._test_dict import DictSize +from rustapi_module.test_dict import DictSize import pytest + @pytest.mark.parametrize( "size", [64, 128, 256], diff --git a/src/types/dict.rs b/src/types/dict.rs index 9434c7b3..4293d8e8 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -8,7 +8,7 @@ use crate::object::PyObject; use crate::python::{IntoPyPointer, Python, ToPyPointer}; use crate::types::{PyList, PyObjectRef}; use std; -use std::{cmp, collections, hash, mem, ops::Drop}; +use std::{cmp, collections, hash, mem}; /// Represents a Python `dict`. #[repr(transparent)]