Skip to content

_py_abc Python implementation of abc is not thread-safe #130095

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

Open
colesbury opened this issue Feb 13, 2025 · 10 comments
Open

_py_abc Python implementation of abc is not thread-safe #130095

colesbury opened this issue Feb 13, 2025 · 10 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Feb 13, 2025

Bug report

The update to the invalidation counter is not thread-safe and can lose updates in some Python implementations:

Failures seen on:

  • Python 3.14t
  • Python 3.9
  • pypy3.10
  • pypy3.11

But not on Python 3.10-3.14 with GIL due to limited GIL switch opportunities.

cpython/Lib/_py_abc.py

Lines 54 to 70 in 05e89c3

def register(cls, subclass):
"""Register a virtual subclass of an ABC.
Returns the subclass, to allow usage as a class decorator.
"""
if not isinstance(subclass, type):
raise TypeError("Can only register classes")
if issubclass(subclass, cls):
return subclass # Already a subclass
# Subtle: test for cycles *after* testing for "already a subclass";
# this means we allow X.register(X) and interpret it as a no-op.
if issubclass(cls, subclass):
# This would create a cycle, which is bad for the algorithm below
raise RuntimeError("Refusing to create an inheritance cycle")
cls._abc_registry.add(subclass)
ABCMeta._abc_invalidation_counter += 1 # Invalidate negative cache
return subclass

For example, consider the following repro, adapted from test_abc.test_registration_basics:

import _py_abc as abc # Use Python implementation of ABCs!!
import threading
import os
import sys

sys.setswitchinterval(1e-6)

N = 5

def run(b):
    b.wait()

    class A(metaclass=abc.ABCMeta):
        pass
    A.register(int)
    if not isinstance(42, A):
        print("Oops!")
        os._exit(1)

def main():
    for _ in range(10000):
        threads = []
        b = threading.Barrier(N)
        for _ in range(N):
            t = threading.Thread(target=run, args=(b,))
            threads.append(t)
            t.start()
        for t in threads:
            t.join()


if __name__ == "__main__":
    main()

Linked PRs

@colesbury colesbury added the type-bug An unexpected behavior, bug, or error label Feb 13, 2025
@colesbury
Copy link
Contributor Author

I'm not really sure what to do here. We could add a threading.Lock() to protect _abc_invalidation_counter, but I'm not sure we want to import the threading module in _py_abc.py.

cc @cfbolz in case you have thoughts on this.

@encukou encukou added the stdlib Python modules in the Lib dir label Feb 14, 2025
@picnixz
Copy link
Member

picnixz commented Feb 14, 2025

Not sure if this is relevant: https://github.com/python/cpython/actions/runs/13335219139/job/37248893358?pr=129175 (I don't know if the test is testing the Python or the C implementation).

EDIT: I think this is the reproducer you had. But now we caught it in the CI

colesbury added a commit to colesbury/cpython that referenced this issue Feb 14, 2025
The `_py_abc` implementation is not currently thread-safe (even with the GIL).
Don't run these tests with `--parallel-threads=N` for now.
@colesbury
Copy link
Contributor Author

Thanks @picnixz. I put up a PR to avoid running them with --parallel-threads for now until we figure out if and how to make _py_abc thread-safe.

@cfbolz
Copy link
Contributor

cfbolz commented Feb 14, 2025

yeah, importing threading in _py_abc leads to a circular import problem if _abc is not available. What about using itertools.count? that's supposed to be thread-safe, isn't it?

@cfbolz
Copy link
Contributor

cfbolz commented Feb 14, 2025

hm, doesn't quite work, because itertools.count has no way to read the current value, only a way to increase the count and get the next value.

@sergey-miryanov
Copy link
Contributor

If I'm right, the problem comes from three bytecode operations: LOAD_ATTR, BINARY_OP, and STORE_ATTR. I think there are two ways to fix this: using a specialized operation for atomic increment/update or the same built-in function. Current main branch has only three uses of _Py_atomic_add_uint64 - qsbr, _asynciomodule and _abc, so there are no existing ways to do atomic update from python. I believe the quickest way to add private built-in for this, but the right one to add atomic ops.

Sorry if I'm obvious.

@colesbury
Copy link
Contributor Author

The pure Python companion modules like _py_abc aren't really used by CPython in normal circumstances outside of tests. They are primarily used by other Python implementations like PyPy. I don't think it makes much sense to consider CPython specific changes to bytecode.

https://peps.python.org/pep-0399/

@sergey-miryanov
Copy link
Contributor

Thanks for the link - it is useful for my understanding.
I’m stick to atomic ops, but I’m sure you have much more and deeper understanding.

colesbury added a commit that referenced this issue Feb 14, 2025
The `_py_abc` implementation is not currently thread-safe (even with the GIL).
Don't run these tests with `--parallel-threads=N` for now.
@cfbolz
Copy link
Contributor

cfbolz commented Feb 14, 2025

@colesbury if you are specifically concerned about pypy, we don't use _py_abc.py anymore either.

@colesbury
Copy link
Contributor Author

Thanks for the clarification. I also noticed a failure when I substituted abc for _py_abc with pypy311, so there might be a thread-safety issue there as well.

If neither pypy nor CPython use _py_abc outside of tests, I'm not going to spend much more time on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants