Merge pull request #2554 from mejrs/acquire_gil2

Deprecate acquire_gil

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)

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(())
 }

pytests/tests/test_misc.py

@@ -8,6 +8,7 @@
 def test_issue_219():
     # Should not deadlock
     pyo3_pytests.misc.issue_219()
+    pyo3_pytests.misc.issue_219_2()
 
 
 @pytest.mark.skipif(

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());
 

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);

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();
 

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);
+    });
 }

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> = PyDict::new(gil.python()).into();
     let dict: &PyDict = dict.as_ref(gil.python());

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