From 13380205113a534bdd7174208045923acd99ca1c Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Mon, 11 Sep 2023 15:20:39 +0200 Subject: [PATCH 1/3] relax multiple-import check to only prevent subinterpreters --- newsfragments/3446.changed.md | 1 + pyo3-build-config/src/lib.rs | 5 +++++ pytests/tests/test_misc.py | 24 +++++++++++++++++------ src/impl_/pymodule.rs | 37 +++++++++++++++++++++++++++-------- 4 files changed, 53 insertions(+), 14 deletions(-) create mode 100644 newsfragments/3446.changed.md diff --git a/newsfragments/3446.changed.md b/newsfragments/3446.changed.md new file mode 100644 index 00000000000..089947ff275 --- /dev/null +++ b/newsfragments/3446.changed.md @@ -0,0 +1 @@ +Relax multiple import check to only prevent use of subinterpreters. diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 05320add478..2f5ff4662a9 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -146,6 +146,11 @@ pub fn print_feature_cfgs() { if rustc_minor_version >= 59 { println!("cargo:rustc-cfg=thread_local_const_init"); } + + // Enable use of OnceLock on Rust 1.70 and greater + if rustc_minor_version >= 70 { + println!("cargo:rustc-cfg=once_lock"); + } } /// Private exports used in PyO3's build.rs diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index 8dfd06ba298..226b89d87a6 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -10,15 +10,27 @@ def test_issue_219(): pyo3_pytests.misc.issue_219() +def test_multiple_imports_same_interpreter_ok(): + spec = importlib.util.find_spec("pyo3_pytests.pyo3_pytests") + + module = importlib.util.module_from_spec(spec) + assert dir(module) == dir(pyo3_pytests.pyo3_pytests) + + @pytest.mark.skipif( platform.python_implementation() == "PyPy", - reason="PyPy does not reinitialize the module (appears to be some internal caching)", + reason="PyPy does not support subinterpreters", ) -def test_second_module_import_fails(): - spec = importlib.util.find_spec("pyo3_pytests.pyo3_pytests") +def test_import_in_subinterpreter_forbidden(): + import _xxsubinterpreters + sub_interpreter = _xxsubinterpreters.create() with pytest.raises( - ImportError, - match="PyO3 modules may only be initialized once per interpreter process", + _xxsubinterpreters.RunFailedError, + match="PyO3 modules do not yet support subinterpreters, see https://github.com/PyO3/pyo3/issues/576", ): - importlib.util.module_from_spec(spec) + _xxsubinterpreters.run_string( + sub_interpreter, "import pyo3_pytests.pyo3_pytests" + ) + + _xxsubinterpreters.destroy(sub_interpreter) diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 2572d431e2c..bebc1156d8d 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,9 +1,11 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. -use std::{ - cell::UnsafeCell, - sync::atomic::{self, AtomicBool}, -}; +use std::cell::UnsafeCell; +#[cfg(once_lock)] +use std::sync::OnceLock; + +#[cfg(not(once_lock))] +use parking_lot::Mutex; use crate::{exceptions::PyImportError, ffi, types::PyModule, Py, PyResult, Python}; @@ -12,7 +14,10 @@ pub struct ModuleDef { // wrapped in UnsafeCell so that Rust compiler treats this as interior mutability ffi_def: UnsafeCell, initializer: ModuleInitializer, - initialized: AtomicBool, + #[cfg(once_lock)] + interpreter: OnceLock, + #[cfg(not(once_lock))] + interpreter: Mutex>, } /// Wrapper to enable initializer to be used in const fns. @@ -51,7 +56,10 @@ impl ModuleDef { ModuleDef { ffi_def, initializer, - initialized: AtomicBool::new(false), + #[cfg(once_lock)] + interpreter: OnceLock::new(), + #[cfg(not(once_lock))] + interpreter: Mutex::new(None), } } /// Builds a module using user given initializer. Used for [`#[pymodule]`][crate::pymodule]. @@ -74,9 +82,22 @@ impl ModuleDef { let module = unsafe { Py::::from_owned_ptr_or_err(py, ffi::PyModule_Create(self.ffi_def.get()))? }; - if self.initialized.swap(true, atomic::Ordering::SeqCst) { + let current_interpreter = + unsafe { ffi::PyInterpreterState_GetID(ffi::PyInterpreterState_Get()) }; + let initialized_interpreter = py.allow_threads(|| { + #[cfg(once_lock)] + { + *self.interpreter.get_or_init(|| current_interpreter) + } + + #[cfg(not(once_lock))] + { + *self.interpreter.lock().get_or_insert(current_interpreter) + } + }); + if current_interpreter != initialized_interpreter { return Err(PyImportError::new_err( - "PyO3 modules may only be initialized once per interpreter process", + "PyO3 modules do not yet support subinterpreters, see https://github.com/PyO3/pyo3/issues/576", )); } (self.initializer.0)(py, module.as_ref(py))?; From f17e70316751285340508d0009103570af7e0873 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 14 Sep 2023 14:40:43 +0200 Subject: [PATCH 2/3] return existing module on Python 3.9 and up --- guide/src/building_and_distribution.md | 2 +- newsfragments/3446.changed.md | 2 +- pyo3-build-config/src/lib.rs | 5 -- pytests/tests/test_misc.py | 9 +++ src/impl_/pymodule.rs | 87 +++++++++++++++++--------- 5 files changed, 67 insertions(+), 38 deletions(-) diff --git a/guide/src/building_and_distribution.md b/guide/src/building_and_distribution.md index 0aee432c2d0..8cb675f4303 100644 --- a/guide/src/building_and_distribution.md +++ b/guide/src/building_and_distribution.md @@ -276,7 +276,7 @@ If you encounter these or other complications when linking the interpreter stati ### Import your module when embedding the Python interpreter -When you run your Rust binary with an embedded interpreter, any `#[pymodule]` created modules won't be accessible to import unless added to a table called `PyImport_Inittab` before the embedded interpreter is initialized. This will cause Python statements in your embedded interpreter such as `import your_new_module` to fail. You can call the macro [`append_to_inittab`]({{#PYO3_DOCS_URL}}/pyo3/macro.append_to_inittab.html) with your module before initializing the Python interpreter to add the module function into that table. (The Python interpreter will be initialized by calling `prepare_freethreaded_python`, `with_embedded_interpreter`, or `Python::with_gil` with the [`auto-initialize`](features.md#auto-initialize) feature enabled.) +When you run your Rust binary with an embedded interpreter, any `#[pymodule]` created modules won't be accessible to import unless added to a table called `PyImport_Inittab` before the embedded interpreter is initialized. This will cause Python statements in your embedded interpreter such as `import your_new_module` to fail. You can call the macro [`append_to_inittab`]({{#PYO3_DOCS_URL}}/pyo3/macro.append_to_inittab.html) with your module before initializing the Python interpreter to add the module function into that table. (The Python interpreter will be initialized by calling `prepare_freethreaded_python`, `with_embedded_python_interpreter`, or `Python::with_gil` with the [`auto-initialize`](features.md#auto-initialize) feature enabled.) ## Cross Compiling diff --git a/newsfragments/3446.changed.md b/newsfragments/3446.changed.md index 089947ff275..a258fb4af67 100644 --- a/newsfragments/3446.changed.md +++ b/newsfragments/3446.changed.md @@ -1 +1 @@ -Relax multiple import check to only prevent use of subinterpreters. +`#[pymodule]` will now return the same module object on repeated import by the same Python interpreter, on Python 3.9 and up. diff --git a/pyo3-build-config/src/lib.rs b/pyo3-build-config/src/lib.rs index 2f5ff4662a9..05320add478 100644 --- a/pyo3-build-config/src/lib.rs +++ b/pyo3-build-config/src/lib.rs @@ -146,11 +146,6 @@ pub fn print_feature_cfgs() { if rustc_minor_version >= 59 { println!("cargo:rustc-cfg=thread_local_const_init"); } - - // Enable use of OnceLock on Rust 1.70 and greater - if rustc_minor_version >= 70 { - println!("cargo:rustc-cfg=once_lock"); - } } /// Private exports used in PyO3's build.rs diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index 226b89d87a6..c3e03c04b09 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -1,5 +1,6 @@ import importlib import platform +import sys import pyo3_pytests.misc import pytest @@ -10,6 +11,10 @@ def test_issue_219(): pyo3_pytests.misc.issue_219() +@pytest.mark.xfail( + platform.python_implementation() == "CPython" and sys.version_info < (3, 9), + reason="Cannot identify subinterpreters on Python older than 3.9", +) def test_multiple_imports_same_interpreter_ok(): spec = importlib.util.find_spec("pyo3_pytests.pyo3_pytests") @@ -17,6 +22,10 @@ def test_multiple_imports_same_interpreter_ok(): assert dir(module) == dir(pyo3_pytests.pyo3_pytests) +@pytest.mark.xfail( + platform.python_implementation() == "CPython" and sys.version_info < (3, 9), + reason="Cannot identify subinterpreters on Python older than 3.9", +) @pytest.mark.skipif( platform.python_implementation() == "PyPy", reason="PyPy does not support subinterpreters", diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index bebc1156d8d..522341287c6 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,23 +1,24 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. use std::cell::UnsafeCell; -#[cfg(once_lock)] -use std::sync::OnceLock; -#[cfg(not(once_lock))] -use parking_lot::Mutex; +#[cfg(all(not(PyPy), Py_3_9))] +use std::sync::atomic::{AtomicI64, Ordering}; -use crate::{exceptions::PyImportError, ffi, types::PyModule, Py, PyResult, Python}; +#[cfg(not(PyPy))] +use crate::exceptions::PyImportError; +use crate::{ffi, sync::GILOnceCell, types::PyModule, Py, PyResult, Python}; /// `Sync` wrapper of `ffi::PyModuleDef`. pub struct ModuleDef { // wrapped in UnsafeCell so that Rust compiler treats this as interior mutability ffi_def: UnsafeCell, initializer: ModuleInitializer, - #[cfg(once_lock)] - interpreter: OnceLock, - #[cfg(not(once_lock))] - interpreter: Mutex>, + /// Interpreter ID where module was initialized (not applicable on PyPy). + #[cfg(all(not(PyPy), Py_3_9))] + interpreter: AtomicI64, + /// Initialized module object, cached to avoid reinitialization. + module: GILOnceCell>, } /// Wrapper to enable initializer to be used in const fns. @@ -56,10 +57,10 @@ impl ModuleDef { ModuleDef { ffi_def, initializer, - #[cfg(once_lock)] - interpreter: OnceLock::new(), - #[cfg(not(once_lock))] - interpreter: Mutex::new(None), + // -1 is never expected to be a valid interpreter ID + #[cfg(all(not(PyPy), Py_3_9))] + interpreter: AtomicI64::new(-1), + module: GILOnceCell::new(), } } /// Builds a module using user given initializer. Used for [`#[pymodule]`][crate::pymodule]. @@ -79,29 +80,53 @@ impl ModuleDef { ))?; } } - let module = unsafe { - Py::::from_owned_ptr_or_err(py, ffi::PyModule_Create(self.ffi_def.get()))? - }; - let current_interpreter = - unsafe { ffi::PyInterpreterState_GetID(ffi::PyInterpreterState_Get()) }; - let initialized_interpreter = py.allow_threads(|| { - #[cfg(once_lock)] + // Check the interpreter ID has not changed, since we currently have no way to guarantee + // that static data is not reused across interpreters. + // + // PyPy does not have subinterpreters, so no need to check interpreter ID. + #[cfg(not(PyPy))] + { + #[cfg(Py_3_9)] { - *self.interpreter.get_or_init(|| current_interpreter) + let current_interpreter = + unsafe { ffi::PyInterpreterState_GetID(ffi::PyInterpreterState_Get()) }; + crate::err::error_on_minusone(py, current_interpreter)?; + if let Err(initialized_interpreter) = self.interpreter.compare_exchange( + -1, + current_interpreter, + Ordering::SeqCst, + Ordering::SeqCst, + ) { + if initialized_interpreter != current_interpreter { + return Err(PyImportError::new_err( + "PyO3 modules do not yet support subinterpreters, see https://github.com/PyO3/pyo3/issues/576", + )); + } + } } - - #[cfg(not(once_lock))] + #[cfg(not(Py_3_9))] { - *self.interpreter.lock().get_or_insert(current_interpreter) + // CPython before 3.9 does not have APIs to check the interpreter ID, so best that can be + // done to guard against subinterpreters is fail if the module is initialized twice + if self.module.get(py).is_some() { + return Err(PyImportError::new_err( + "PyO3 modules compiled for CPython 3.8 or older may only be initialized once per interpreter process" + )); + } } - }); - if current_interpreter != initialized_interpreter { - return Err(PyImportError::new_err( - "PyO3 modules do not yet support subinterpreters, see https://github.com/PyO3/pyo3/issues/576", - )); } - (self.initializer.0)(py, module.as_ref(py))?; - Ok(module) + self.module + .get_or_try_init(py, || { + let module = unsafe { + Py::::from_owned_ptr_or_err( + py, + ffi::PyModule_Create(self.ffi_def.get()), + )? + }; + (self.initializer.0)(py, module.as_ref(py))?; + Ok(module) + }) + .map(|py_module| py_module.clone_ref(py)) } } From 1a349c2eb70f15cf7dff099937c26ba9e1dcfb99 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:08:47 +0100 Subject: [PATCH 3/3] adjust cfgs for windows 3.9 --- pytests/tests/test_misc.py | 7 ++++++- src/impl_/pymodule.rs | 12 +++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index c3e03c04b09..9cc0cebc771 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -33,10 +33,15 @@ def test_multiple_imports_same_interpreter_ok(): def test_import_in_subinterpreter_forbidden(): import _xxsubinterpreters + if sys.version_info < (3, 12): + expected_error = "PyO3 modules do not yet support subinterpreters, see https://github.com/PyO3/pyo3/issues/576" + else: + expected_error = "module pyo3_pytests.pyo3_pytests does not support loading in subinterpreters" + sub_interpreter = _xxsubinterpreters.create() with pytest.raises( _xxsubinterpreters.RunFailedError, - match="PyO3 modules do not yet support subinterpreters, see https://github.com/PyO3/pyo3/issues/576", + match=expected_error, ): _xxsubinterpreters.run_string( sub_interpreter, "import pyo3_pytests.pyo3_pytests" diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 522341287c6..8ec963455fc 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -2,7 +2,7 @@ use std::cell::UnsafeCell; -#[cfg(all(not(PyPy), Py_3_9))] +#[cfg(all(not(PyPy), Py_3_9, not(all(windows, Py_LIMITED_API, not(Py_3_10)))))] use std::sync::atomic::{AtomicI64, Ordering}; #[cfg(not(PyPy))] @@ -15,7 +15,7 @@ pub struct ModuleDef { ffi_def: UnsafeCell, initializer: ModuleInitializer, /// Interpreter ID where module was initialized (not applicable on PyPy). - #[cfg(all(not(PyPy), Py_3_9))] + #[cfg(all(not(PyPy), Py_3_9, not(all(windows, Py_LIMITED_API, not(Py_3_10)))))] interpreter: AtomicI64, /// Initialized module object, cached to avoid reinitialization. module: GILOnceCell>, @@ -58,7 +58,7 @@ impl ModuleDef { ffi_def, initializer, // -1 is never expected to be a valid interpreter ID - #[cfg(all(not(PyPy), Py_3_9))] + #[cfg(all(not(PyPy), Py_3_9, not(all(windows, Py_LIMITED_API, not(Py_3_10)))))] interpreter: AtomicI64::new(-1), module: GILOnceCell::new(), } @@ -86,7 +86,9 @@ impl ModuleDef { // PyPy does not have subinterpreters, so no need to check interpreter ID. #[cfg(not(PyPy))] { - #[cfg(Py_3_9)] + // PyInterpreterState_Get is only available on 3.9 and later, but is missing + // from python3.dll for Windows stable API on 3.9 + #[cfg(all(Py_3_9, not(all(windows, Py_LIMITED_API, not(Py_3_10)))))] { let current_interpreter = unsafe { ffi::PyInterpreterState_GetID(ffi::PyInterpreterState_Get()) }; @@ -104,7 +106,7 @@ impl ModuleDef { } } } - #[cfg(not(Py_3_9))] + #[cfg(not(all(Py_3_9, not(all(windows, Py_LIMITED_API, not(Py_3_10))))))] { // CPython before 3.9 does not have APIs to check the interpreter ID, so best that can be // done to guard against subinterpreters is fail if the module is initialized twice