-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
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). |
It looks like the existing
That said, I don't see any explicitly multithreaded tests in the existing test suite. The tests run using There are some
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 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 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. |
I don't believe any locks are required. All of the places where we have
mutable data (e.g., Hash, CipherContext) only mutate with &mut references,
which pyo3 ensures are exclusive.
There's no coherent behavior for two threads to update any of these objects
concurrently without their own synchronization, so its fine IMO that those
simply raise.
…On Mon, Feb 24, 2025 at 1:16 PM Nathan Goldbaum ***@***.***> wrote:
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
<#12488 (comment)>
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 <#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.
—
Reply to this email directly, view it on GitHub
<#12489 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBGE36EVNONJMUTEFKL2RNOYLAVCNFSM6AAAAABXPKF7YOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZZGI4DQMJYGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: ngoldbaum]*ngoldbaum* left a comment (pyca/cryptography#12489)
<#12489 (comment)>
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
<#12488 (comment)>
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 <#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.
—
Reply to this email directly, view it on GitHub
<#12489 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBGE36EVNONJMUTEFKL2RNOYLAVCNFSM6AAAAABXPKF7YOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNZZGI4DQMJYGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
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 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 Thanks so much for your help and advice 😀. |
I don't feel any particular need to have tests for all the immutable objects.
…On Mon, Feb 24, 2025 at 1:52 PM Nathan Goldbaum ***@***.***> wrote:
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 😀.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
ngoldbaum left a comment (pyca/cryptography#12489)
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 😀.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
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.
The text was updated successfully, but these errors were encountered: