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 Python 3.13 #925

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ngoldbaum
Copy link
Contributor

Towards fixing #913.

  • Adds pytest-run-parallel as a test dependency. This does a good job of detecting thread-unsafe use of global state but doesn't catch thread safety issues due to sharing mutable state stored in objects. I marked one test as thread-unsafe because it uses the warnings module, which isn't thread-safe.

  • Declares that the rust PyO3 module exprted in _bcrypt doesn't use the GIL. It looks like the rust code exports python functions and doesn't define any objects with state. Am I missing something?

  • Sets up free-threaded CI using quansight-labs/setup-python.

It looks like the existing tests don't use threading at all. Should I add some multithreaded tests? Or is pytest-run-parallel executing the existing tests in parallel sufficient?

After this I'll look at updating the wheel-building and uploading 313t wheels.

@ngoldbaum ngoldbaum force-pushed the support-free-threading branch from dcb6071 to 1d8f655 Compare November 25, 2024 19:39
@ngoldbaum
Copy link
Contributor Author

ngoldbaum commented Nov 25, 2024

It looks like the lack of Python3.7 support in pytest-run-parallel is a little annoying to deal with in tox (or at least I don't see how to deal with it straightforwardly given that we need to make a code modification to the tests).

Let me see how hard it is to support Python 3.7 in pytest-run-parallel.

@alex
Copy link
Member

alex commented Nov 25, 2024 via email

@reaperhulk
Copy link
Member

I think we probably do want some explicit threading testing separate from the use of threading in the test harness itself. A quick lazy glance at the stats suggests we're ~5-7% Python 3.7 downloads per day. On pyca/cryptography that'd be too high, but this project sees less change so I think I'd be okay with dropping it.

@ngoldbaum
Copy link
Contributor Author

I think we probably do want some explicit threading testing separate from the use of threading in the test harness itself.

Can you share a little more what you had in mind here? Maybe several threads hashing and checking passwords based on shared bytes or hashes? Is there any state stored anywhere? It looks like everything is a pure function.

@ngoldbaum
Copy link
Contributor Author

I think I'd be okay with dropping it.

Great, I'll look at dropping Python 3.7 in a separate PR.

@reaperhulk
Copy link
Member

Can you share a little more what you had in mind here? Maybe several threads hashing and checking passwords based on shared bytes or hashes? Is there any state stored anywhere? It looks like everything is a pure function.

Yeah there's no state, but your proposed idea is what I'd do. Just mimicking what a multi-threaded workload would look like with hash gen, verification, and kdf operations.

@ngoldbaum ngoldbaum force-pushed the support-free-threading branch from fed9d38 to ba2bcd4 Compare November 26, 2024 19:37
@@ -219,7 +224,7 @@ def test_gensalt_bad_prefix():
bcrypt.gensalt(prefix=b"bad")


def test_gensalt_2a_prefix(monkeypatch):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR, but I noticed these fixtures weren't being used anymore

@ngoldbaum
Copy link
Contributor Author

I added a test that uses threading, let me know if you have any suggestions.

It also looks like PyO3 0.23.2 might have broken setuptools-rust on Windows?

@ngoldbaum
Copy link
Contributor Author

@davidhewitt
Copy link

I think PyO3/pyo3#4733 might fix it, want to try setting that git branch here and see if resolves?

@ngoldbaum ngoldbaum force-pushed the support-free-threading branch from 994a54e to 9187561 Compare December 6, 2024 17:53
@ngoldbaum
Copy link
Contributor Author

@reaperhulk it looks like all the upstream issues have been sorted out and this is ready now.

self.id_ = id_
self.salt = bcrypt.gensalt(4)
self.hash_ = bcrypt.hashpw(pw, self.salt)
self.key = bcrypt.kdf(pw, self.salt, 32, 50)
Copy link
Contributor Author

@ngoldbaum ngoldbaum Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose these parameters to make this run in a reasonable time. Still, it's a very slow test compared with the rest of the test suite.

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

Successfully merging this pull request may close these issues.

4 participants