From fc6121eafe4c467d9d5bd5d1b5979143382648b5 Mon Sep 17 00:00:00 2001 From: mejrs <> Date: Mon, 15 Aug 2022 03:34:47 +0200 Subject: [PATCH] Deprecate acquire_gil --- CHANGELOG.md | 2 +- pytests/src/misc.rs | 8 ++++++++ pytests/tests/test_misc.py | 1 + src/gil.rs | 14 ++++++++++++++ src/marker.rs | 16 +++++++++++----- tests/test_sequence.rs | 1 + tests/ui/invalid_result_conversion.rs | 6 +++--- tests/ui/wrong_aspyref_lifetimes.rs | 1 + tests/ui/wrong_aspyref_lifetimes.stderr | 18 +++++++++--------- 9 files changed, 49 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e79446c..5b21ff8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,7 +57,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Downcasting (`PyTryFrom`) behavior has changed for `PySequence` and `PyMapping`: classes are now required to inherit from (or register with) the corresponding Python standard library abstract base class. See the [migration guide](https://pyo3.rs/latest/migration.html) for information on fixing broken downcasts. [#2477](https://github.com/PyO3/pyo3/pull/2477) - Disable `PyFunction` on `Py_LIMITED_API` and PyPy. [#2542](https://github.com/PyO3/pyo3/pull/2542) - Panics during drop of panic payload caught by PyO3 will now abort. [#2544](https://github.com/PyO3/pyo3/pull/2544) - +- Deprecate `Python::acquire_gil` [#2549](https://github.com/PyO3/pyo3/pull/2549). ### Removed - Remove all functionality deprecated in PyO3 0.15. [#2283](https://github.com/PyO3/pyo3/pull/2283) diff --git a/pytests/src/misc.rs b/pytests/src/misc.rs index 652f3e82..42ae7029 100644 --- a/pytests/src/misc.rs +++ b/pytests/src/misc.rs @@ -3,12 +3,20 @@ use pyo3::prelude::*; #[pyfunction] fn issue_219() { // issue 219: acquiring GIL inside #[pyfunction] deadlocks. + #[allow(deprecated)] let gil = Python::acquire_gil(); let _py = gil.python(); } +#[pyfunction] +fn issue_219_2() { + // issue 219: acquiring GIL inside #[pyfunction] deadlocks. + Python::with_gil(|_| {}); +} + #[pymodule] pub fn misc(_py: Python<'_>, m: &PyModule) -> PyResult<()> { m.add_function(wrap_pyfunction!(issue_219, m)?)?; + m.add_function(wrap_pyfunction!(issue_219_2, m)?)?; Ok(()) } diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index 8dfd06ba..0449b418 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -8,6 +8,7 @@ import pytest def test_issue_219(): # Should not deadlock pyo3_pytests.misc.issue_219() + pyo3_pytests.misc.issue_219_2() @pytest.mark.skipif( diff --git a/src/gil.rs b/src/gil.rs index 31c28aee..bc1ec0e4 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -151,6 +151,7 @@ where /// use pyo3::Python; /// /// { +/// #[allow(deprecated)] /// let gil_guard = Python::acquire_gil(); /// let py = gil_guard.python(); /// } // GIL is released when gil_guard is dropped @@ -516,6 +517,7 @@ mod tests { #[test] fn test_owned() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); let obj = get_object(py); @@ -541,6 +543,7 @@ mod tests { #[test] fn test_owned_nested() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); let obj = get_object(py); @@ -574,6 +577,7 @@ mod tests { #[test] fn test_pyobject_drop_with_gil_decreases_refcnt() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); let obj = get_object(py); @@ -595,6 +599,7 @@ mod tests { #[test] fn test_pyobject_drop_without_gil_doesnt_decrease_refcnt() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); let obj = get_object(py); @@ -615,6 +620,7 @@ mod tests { { // Next time the GIL is acquired, the object is released + #[allow(deprecated)] let _gil = Python::acquire_gil(); assert_eq!(ffi::Py_REFCNT(obj_ptr), 1); } @@ -627,6 +633,7 @@ mod tests { let get_gil_count = || GIL_COUNT.with(|c| c.get()); assert_eq!(get_gil_count(), 0); + #[allow(deprecated)] let gil = Python::acquire_gil(); assert_eq!(get_gil_count(), 1); @@ -640,6 +647,7 @@ mod tests { drop(pool); assert_eq!(get_gil_count(), 2); + #[allow(deprecated)] let gil2 = Python::acquire_gil(); assert_eq!(get_gil_count(), 3); @@ -656,6 +664,7 @@ mod tests { #[test] fn test_allow_threads() { // allow_threads should temporarily release GIL in PyO3's internal tracking too. + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); @@ -664,6 +673,7 @@ mod tests { py.allow_threads(move || { assert!(!gil_is_acquired()); + #[allow(deprecated)] let gil = Python::acquire_gil(); assert!(gil_is_acquired()); @@ -677,9 +687,11 @@ mod tests { #[test] fn dropping_gil_does_not_invalidate_references() { // Acquiring GIL for the second time should be safe - see #864 + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); + #[allow(deprecated)] let gil2 = Python::acquire_gil(); let obj = py.eval("object()", None, None).unwrap(); drop(gil2); @@ -690,6 +702,7 @@ mod tests { #[test] fn test_clone_with_gil() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); @@ -850,6 +863,7 @@ mod tests { // update_counts can run arbitrary Python code during Py_DECREF. // if the locking is implemented incorrectly, it will deadlock. + #[allow(deprecated)] let gil = Python::acquire_gil(); let obj = get_object(gil.python()); diff --git a/src/marker.rs b/src/marker.rs index e3d48051..7540b615 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -379,9 +379,18 @@ impl<'py> Python<'py> { /// allowed, and will not deadlock. However, [`GILGuard`]s must be dropped in the reverse order /// to acquisition. If PyO3 detects this order is not maintained, it will panic when the out-of-order drop occurs. /// + /// # Deprecation + /// + /// This API has been deprecated for several reasons: + /// - GIL drop order tracking has turned out to be [error prone](https://github.com/PyO3/pyo3/issues/1683). + /// With a scoped API like `Python::with_gil`, these are always dropped in the correct order. + /// - It promotes passing and keeping the GILGuard around, which is almost always not what you actually want. + /// /// [`PyGILState_Ensure`]: crate::ffi::PyGILState_Ensure /// [`auto-initialize`]: https://pyo3.rs/main/features.html#auto-initialize #[inline] + // Once removed, we can remove GILGuard's drop tracking. + #[deprecated(since = "0.17.0", note = "prefer Python::with_gil")] pub fn acquire_gil() -> GILGuard { GILGuard::acquire() } @@ -1010,13 +1019,10 @@ mod tests { let state = unsafe { crate::ffi::PyGILState_Check() }; assert_eq!(state, GIL_NOT_HELD); - { - let gil = Python::acquire_gil(); - let _py = gil.python(); + Python::with_gil(|_| { let state = unsafe { crate::ffi::PyGILState_Check() }; assert_eq!(state, GIL_HELD); - drop(gil); - } + }); let state = unsafe { crate::ffi::PyGILState_Check() }; assert_eq!(state, GIL_NOT_HELD); diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index f2aaa078..25dddce6 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -315,6 +315,7 @@ fn test_option_list_get() { #[test] fn sequence_is_not_mapping() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let py = gil.python(); diff --git a/tests/ui/invalid_result_conversion.rs b/tests/ui/invalid_result_conversion.rs index 53770248..4cf3e0bd 100644 --- a/tests/ui/invalid_result_conversion.rs +++ b/tests/ui/invalid_result_conversion.rs @@ -24,7 +24,7 @@ fn should_not_work() -> Result<(), MyError> { } fn main() { - let gil = Python::acquire_gil(); - let py = gil.python(); - wrap_pyfunction!(should_not_work)(py); + Python::with_gil(|py|{ + wrap_pyfunction!(should_not_work)(py); + }); } diff --git a/tests/ui/wrong_aspyref_lifetimes.rs b/tests/ui/wrong_aspyref_lifetimes.rs index 76c7e196..98d8adbf 100644 --- a/tests/ui/wrong_aspyref_lifetimes.rs +++ b/tests/ui/wrong_aspyref_lifetimes.rs @@ -1,6 +1,7 @@ use pyo3::{types::PyDict, Py, Python}; fn main() { + #[allow(deprecated)] let gil = Python::acquire_gil(); let dict: Py = PyDict::new(gil.python()).into(); let dict: &PyDict = dict.as_ref(gil.python()); diff --git a/tests/ui/wrong_aspyref_lifetimes.stderr b/tests/ui/wrong_aspyref_lifetimes.stderr index 0042bc6b..7044250e 100644 --- a/tests/ui/wrong_aspyref_lifetimes.stderr +++ b/tests/ui/wrong_aspyref_lifetimes.stderr @@ -1,10 +1,10 @@ error[E0505]: cannot move out of `gil` because it is borrowed - --> tests/ui/wrong_aspyref_lifetimes.rs:7:10 - | -6 | let dict: &PyDict = dict.as_ref(gil.python()); - | ------------ borrow of `gil` occurs here -7 | drop(gil); - | ^^^ move out of `gil` occurs here -8 | -9 | let _py: Python = dict.py(); // Obtain a Python<'p> without GIL. - | --------- borrow later used here + --> tests/ui/wrong_aspyref_lifetimes.rs:8:10 + | +7 | let dict: &PyDict = dict.as_ref(gil.python()); + | ------------ borrow of `gil` occurs here +8 | drop(gil); + | ^^^ move out of `gil` occurs here +9 | +10 | let _py: Python = dict.py(); // Obtain a Python<'p> without GIL. + | --------- borrow later used here