Skip to content

Conversation

@swtaarrs
Copy link
Member

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

@swtaarrsswtaarrs requested a review from colesburyApril 2, 2024 23:09
@swtaarrsswtaarrs marked this pull request as ready for review April 3, 2024 18:30
Copy link
Contributor

@colesburycolesbury 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.

@swtaarrs
Copy link
MemberAuthor

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
MemberAuthor

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

@colesburycolesbury merged commit f268e32 into python:mainApr 11, 2024
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@swtaarrs@colesbury