Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support free-threaded CPython (3.13t) #12489

Open
ngoldbaum opened this issue Feb 19, 2025 · 5 comments
Open

Support free-threaded CPython (3.13t) #12489

ngoldbaum opened this issue Feb 19, 2025 · 5 comments

Comments

@ngoldbaum
Copy link
Contributor

We have a WIP version of cffi that builds on the free-threaded interpreter: Quansight-Labs/cffi#1

I think with that it should now be possible to make some headway on free-threaded support without too much hacking to disable things.

Unfortunately we're still blocked on releasing cp313t wheels to users until CFFI support is merged, unless there's any appetite to depend on our fork. There isn't a way to depend on the fork only on the free-threaded build because environment markers don't know about ABI tags. We're hoping to have a PEP and a fix for that in Python 3.14 but that doesn't really help much right now or in the next few years.

@alex
Copy link
Member

alex commented Feb 19, 2025

At the current juncture, we don't have an appetite to depend on a cffi fork.

We're more interested in burning down our usage of cffi (which at this point exists only for pyopenssl).

@ngoldbaum
Copy link
Contributor Author

It looks like the existing nox test sessions all pass on the free-threaded build, at least locally on my mac, after applying the following patch:

diff --git a/pyproject.toml b/pyproject.toml
index 02d58b00a..ed0ba51d9 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -5,7 +5,7 @@ requires = [
     "maturin>=1,<2",

     # Must be kept in sync with `project.dependencies`
-    "cffi>=1.12; platform_python_implementation != 'PyPy'",
+    "cffi @ git+https://github.com/quansight-labs/cffi",
     # Used by cffi (which import distutils, and in Python 3.12, distutils has
     # been removed from the stdlib, but installing setuptools puts it back) as
     # well as our build.rs for the rust/cffi bridge.
diff --git a/src/rust/cryptography-cffi/build.rs b/src/rust/cryptography-cffi/build.rs
index 1243a8187..2ddb455f4 100644
--- a/src/rust/cryptography-cffi/build.rs
+++ b/src/rust/cryptography-cffi/build.rs
@@ -78,8 +78,15 @@ fn main() {
         build.include(python_include);
     }

-    // Enable abi3 mode if we're not using PyPy.
-    if python_impl != "PyPy" {
+    let is_free_threaded = run_python_script(
+        &python,
+        "import sysconfig; print(bool(sysconfig.get_config_var('Py_GIL_DISABLED')), end='')",
+    )
+    .unwrap()
+        == "True";
+
+    // Enable abi3 mode if we're not using PyPy or the free-threaded build
+    if !(python_impl == "PyPy" || is_free_threaded) {
         // cp37 (Python 3.7 to help our grep when we some day drop 3.7 support)
         build.define("Py_LIMITED_API", "0x030700f0");
     }

That said, I don't see any explicitly multithreaded tests in the existing test suite. The tests run using pytest-xdist, but that's processed-based parallelism. It looks like there's some code to control multithreaded parallelism inside OpenSSL, but I don't see any other uses of std::thread in Rust code.

There are some unsafe impl Send and Sync uses:

src/rust/cryptography-openssl/src/hmac.rs
21:unsafe impl Sync for Hmac {}
23:unsafe impl Send for Hmac {}

src/rust/cryptography-openssl/src/aead.rs
25:unsafe impl Sync for AeadCtx {}
27:unsafe impl Send for AeadCtx {}

src/rust/cryptography-openssl/src/cmac.rs
22:unsafe impl Sync for Cmac {}
24:unsafe impl Send for Cmac {}

These are all wrappers for C types in OpenSSL. I don't know offhand if the GIL was somehow providing the guarantees backing these implementations.

There is some use of static, but they're all either const or one-time initialized GILOnceCell instances or types backed by GILOnceCell. I don't see any mutable statics.

I see here that I should expect immutable objects to be thread-safe by construction but that currently the only guarantees for objects with state are that they should not be concurrently used between threads.

Rather than adding locking, which might introduce scaling problems, one thing to consider is adding an atomic flag that a thread sets when it is actively using an object with mutable state. If another thread tries to use the object and sees the flag set, then you could generate a runtime error indicating the user tried to something unsupported. It might also just be necessary to document more fully that state should not be shared between threads, e.g. #12488.

It's not clear to me to what extent it's possible to trigger undefined behavior in the GIL-enabled build right now with the threading module if you do share stateful objects between threads.

I'm curious if the maintainers have an opinion about what they would like to see here in terms of testing and adding runtime guardrails or locking to improve thread safety.

@alex
Copy link
Member

alex commented Feb 24, 2025 via email

@ngoldbaum
Copy link
Contributor Author

so its fine IMO that those simply raise

Cool, I will take a look to verify this happens right now via PyO3's runtime borrow checking by adding some explicitly multithreaded tests of stateful primitives.

It looks like the python APIs that provide stateful objects with an update method are CipherContext, HashContext, and PaddingContext, so any concurrent shared use of those with python's threading module should be an error. Are there any other stateful primitives exposed to Python I'm missing?

For the rest of the API that we expect to be thread-safe by construction, how far do you want me to go adding tests to verify that? Are there any high-level integration tests I could leverage to modify into a multithreaded test case along the lines of what was added to bcrypt when we added free-threaded support?

Thanks so much for your help and advice 😀.

@alex
Copy link
Member

alex commented Feb 24, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants