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

gh-116738: Make _abc module thread-safe #117488

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

swtaarrs
Copy link
Member

@swtaarrs swtaarrs commented Apr 2, 2024

This is a collection of small changes aimed at making the _abc module safe to use in a free-threaded build:

  • Only read and write _abcmodule_state.abc_invalidation_counter and _abc_data._abc_negative_cache_version with atomic operations (except in situations when the object should only be visible to a single thread, like initialization, teardown, or GC traverse).

  • Change the two members above from unsigned long long to uint64_t. This was partially to avoid having to add more _Py_atomic_*_ullong variants, but also because unsigned long long is guaranteed to be at least 64 bits and I can't imagine we'd ever want more than 64. Might as well make it explicit.

  • Change _in_weak_set() and _add_to_weak_set() to both take an _abc_data * and PyObject **, to allow them to use critical sections when reading or writing the pointers to sets of weak references. None of the PyObject *s that hold a set will change once they're first initialized, so we don't need to use locking when operating on the sets, only when reading or initializing the pointers.

  • For the most part, no locks are held around multiple operations on related data structures (_abc__abc_subclasscheck_impl() being a good example). User code that does things like performing ABC subclass checks while concurrently registering subclasses will always be subject to surprising results no matter what we do internally, so the module only ensures that its data structures are kept internally consistent.

  • Two functions modify a type's tp_flags member: _abc__abc_init() (should only be called with new types not visible to other threads) and _abc__abc_register_impl() (called with existing types). I added a helper to typeobject.c to support the _abc_register case, and it was just a few more lines to also support the _abc_init case as well. This felt a bit heavy-handed so I'm open to suggestions.

  • Issue: Audit all built-in modules for thread safety #116738

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Overall looks good. A few comments below.

Modules/_abc.c Outdated Show resolved Hide resolved
Modules/_abc.c Outdated Show resolved Hide resolved
@swtaarrs
Copy link
Member Author

Don't merge this yet - I'm still going to make a small improvement to the refcounting of flags (hopefully after my next meeting)

@swtaarrs
Copy link
Member Author

Ok, this should be good to go once the builds finish.

@colesbury colesbury merged commit f268e32 into python:main Apr 11, 2024
34 checks passed
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
A collection of small changes aimed at making the `_abc` module safe to
use in a free-threaded build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants