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<ffi::PyModuleDef>,
initializer: ModuleInitializer,
- initialized: AtomicBool,
+ #[cfg(once_lock)]
+ interpreter: OnceLock<i64>,
+ #[cfg(not(once_lock))]
+ interpreter: Mutex<Option<i64>>,
}
/// 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::<PyModule>::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<ffi::PyModuleDef>,
initializer: ModuleInitializer,
- #[cfg(once_lock)]
- interpreter: OnceLock<i64>,
- #[cfg(not(once_lock))]
- interpreter: Mutex<Option<i64>>,
+ /// 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<Py<PyModule>>,
}
/// 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::<PyModule>::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::<PyModule>::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<ffi::PyModuleDef>,
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<Py<PyModule>>,
@@ -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