Make `_threadmodule.c` thread-safe in `--disable-gil` builds #114271

mpage · 2024-01-18T23:25:52Z

Feature or enhancement

Proposal:

Make the functionality in _threadmodule.c thread-safe in free-threaded builds.

Commits from nogil-3.12: https://github.com/colesbury/nogil-3.12/commits/nogil-3.12/Modules/_threadmodule.c

Context from @colesbury:

Ignore the CriticalLock commit; this was an idea that didn't work and was prone to deadlock.
Ignore the weakrefs change.
Unclear if we need to deal with _tstate_lock.
For the other changes, use your judgement. You may be able to put together smaller changes
_Py_ThreadId() is currently only available in the free-threaded build, so the recursive lock implementation can probably (?) just stick with the existing PyThread_get_thread_ident().

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

The text was updated successfully, but these errors were encountered:

mpage · 2024-01-19T22:29:18Z

@colesbury - Would you assign this to me, please?

…e-threaded builds (gh-115093) Use atomics to mutate PyInterpreterState.threads.count.

…in free-threaded builds (pythongh-115093) Use atomics to mutate PyInterpreterState.threads.count.

…-cherry-pick-ThreadHandle

…115102) The ID of the owning thread (`rlock_owner`) may be accessed by multiple threads without holding the underlying lock; relaxed atomics are used in place of the previous loads/stores. The number of times that the lock has been acquired (`rlock_count`) is only ever accessed by the thread that holds the lock; we do not need to use atomics to access it.

…uilds (GH-115190) Make `_thread.ThreadHandle` thread-safe in free-threaded builds We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The internal thread handle `detach()` method has been removed.

…ilds (python#115102) The ID of the owning thread (`rlock_owner`) may be accessed by multiple threads without holding the underlying lock; relaxed atomics are used in place of the previous loads/stores. The number of times that the lock has been acquired (`rlock_count`) is only ever accessed by the thread that holds the lock; we do not need to use atomics to access it.

…aded builds (pythonGH-115190) Make `_thread.ThreadHandle` thread-safe in free-threaded builds We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The internal thread handle `detach()` method has been removed.

…in free-threaded builds Previously, the `locked` field was set after releasing the lock. This reverses the order so that the `locked` field is set while the lock is still held. There is still one thread-safety issue where `locked` is checked prior to releasing the lock, however, in practice that will only be an issue when unlocking the lock is contended, which should be rare.

…116433) Previously, the `locked` field was set after releasing the lock. This reverses the order so that the `locked` field is set while the lock is still held. There is still one thread-safety issue where `locked` is checked prior to releasing the lock, however, in practice that will only be an issue when unlocking the lock is contended, which should be rare.

There is a race between when `Thread._tstate_lock` is released[^1] in `Thread._wait_for_tstate_lock()` and when `Thread._stop()` asserts[^2] that it is unlocked. Consider the following execution involving threads A, B, and C: 1. A starts. 2. B joins A, blocking on its `_tstate_lock`. 3. C joins A, blocking on its `_tstate_lock`. 4. A finishes and releases its `_tstate_lock`. 5. B acquires A's `_tstate_lock` in `_wait_for_tstate_lock()`, releases it, but is swapped out before calling `_stop()`. 6. C is scheduled, acquires A's `_tstate_lock` in `_wait_for_tstate_lock()` but is swapped out before releasing it. 7. B is scheduled, calls `_stop()`, which asserts that A's `_tstate_lock` is not held. However, C holds it, so the assertion fails. The race can be reproduced[^3] by inserting sleeps at the appropriate points in the threading code. To do so, run the `repro_join_race.py` from the linked repo. There are two main parts to this PR: 1. `_tstate_lock` is replaced with an event that is attached to `PyThreadState`. The event is set by the runtime prior to the thread being cleared (in the same place that `_tstate_lock` was released). `Thread.join()` blocks waiting for the event to be set. 2. `_PyInterpreterState_WaitForThreads()` provides the ability to wait for all non-daemon threads to exit. To do so, an `is_daemon` predicate was added to `PyThreadState`. This field is set each time a thread is created. `threading._shutdown()` now calls into `_PyInterpreterState_WaitForThreads()` instead of waiting on `_tstate_lock`s. [^1]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1201 [^2]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1115 [^3]: mpage@8194653 --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <[email protected]>

…aded builds (pythonGH-115190) Make `_thread.ThreadHandle` thread-safe in free-threaded builds We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The internal thread handle `detach()` method has been removed.

…lds (python#116433) Previously, the `locked` field was set after releasing the lock. This reverses the order so that the `locked` field is set while the lock is still held. There is still one thread-safety issue where `locked` is checked prior to releasing the lock, however, in practice that will only be an issue when unlocking the lock is contended, which should be rare.

There is a race between when `Thread._tstate_lock` is released[^1] in `Thread._wait_for_tstate_lock()` and when `Thread._stop()` asserts[^2] that it is unlocked. Consider the following execution involving threads A, B, and C: 1. A starts. 2. B joins A, blocking on its `_tstate_lock`. 3. C joins A, blocking on its `_tstate_lock`. 4. A finishes and releases its `_tstate_lock`. 5. B acquires A's `_tstate_lock` in `_wait_for_tstate_lock()`, releases it, but is swapped out before calling `_stop()`. 6. C is scheduled, acquires A's `_tstate_lock` in `_wait_for_tstate_lock()` but is swapped out before releasing it. 7. B is scheduled, calls `_stop()`, which asserts that A's `_tstate_lock` is not held. However, C holds it, so the assertion fails. The race can be reproduced[^3] by inserting sleeps at the appropriate points in the threading code. To do so, run the `repro_join_race.py` from the linked repo. There are two main parts to this PR: 1. `_tstate_lock` is replaced with an event that is attached to `PyThreadState`. The event is set by the runtime prior to the thread being cleared (in the same place that `_tstate_lock` was released). `Thread.join()` blocks waiting for the event to be set. 2. `_PyInterpreterState_WaitForThreads()` provides the ability to wait for all non-daemon threads to exit. To do so, an `is_daemon` predicate was added to `PyThreadState`. This field is set each time a thread is created. `threading._shutdown()` now calls into `_PyInterpreterState_WaitForThreads()` instead of waiting on `_tstate_lock`s. [^1]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1201 [^2]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1115 [^3]: mpage@8194653 --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <[email protected]>

colesbury · 2024-04-10T18:12:02Z

@mpage is this complete?

mpage · 2024-04-10T18:46:01Z

Yep. I filed #117721 to track the follow up work we discussed for _thread.lock.release().

…ilds (python#115102) The ID of the owning thread (`rlock_owner`) may be accessed by multiple threads without holding the underlying lock; relaxed atomics are used in place of the previous loads/stores. The number of times that the lock has been acquired (`rlock_count`) is only ever accessed by the thread that holds the lock; we do not need to use atomics to access it.

…aded builds (pythonGH-115190) Make `_thread.ThreadHandle` thread-safe in free-threaded builds We protect the mutable state of `ThreadHandle` using a `_PyOnceFlag`. Concurrent operations (i.e. `join` or `detach`) on `ThreadHandle` block until it is their turn to execute or an earlier operation succeeds. Once an operation has been applied successfully all future operations complete immediately. The `join()` method is now idempotent. It may be called multiple times but the underlying OS thread will only be joined once. After `join()` succeeds, any future calls to `join()` will succeed immediately. The internal thread handle `detach()` method has been removed.

…lds (python#116433) Previously, the `locked` field was set after releasing the lock. This reverses the order so that the `locked` field is set while the lock is still held. There is still one thread-safety issue where `locked` is checked prior to releasing the lock, however, in practice that will only be an issue when unlocking the lock is contended, which should be rare.

There is a race between when `Thread._tstate_lock` is released[^1] in `Thread._wait_for_tstate_lock()` and when `Thread._stop()` asserts[^2] that it is unlocked. Consider the following execution involving threads A, B, and C: 1. A starts. 2. B joins A, blocking on its `_tstate_lock`. 3. C joins A, blocking on its `_tstate_lock`. 4. A finishes and releases its `_tstate_lock`. 5. B acquires A's `_tstate_lock` in `_wait_for_tstate_lock()`, releases it, but is swapped out before calling `_stop()`. 6. C is scheduled, acquires A's `_tstate_lock` in `_wait_for_tstate_lock()` but is swapped out before releasing it. 7. B is scheduled, calls `_stop()`, which asserts that A's `_tstate_lock` is not held. However, C holds it, so the assertion fails. The race can be reproduced[^3] by inserting sleeps at the appropriate points in the threading code. To do so, run the `repro_join_race.py` from the linked repo. There are two main parts to this PR: 1. `_tstate_lock` is replaced with an event that is attached to `PyThreadState`. The event is set by the runtime prior to the thread being cleared (in the same place that `_tstate_lock` was released). `Thread.join()` blocks waiting for the event to be set. 2. `_PyInterpreterState_WaitForThreads()` provides the ability to wait for all non-daemon threads to exit. To do so, an `is_daemon` predicate was added to `PyThreadState`. This field is set each time a thread is created. `threading._shutdown()` now calls into `_PyInterpreterState_WaitForThreads()` instead of waiting on `_tstate_lock`s. [^1]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1201 [^2]: https://github.com/python/cpython/blob/441affc9e7f419ef0b68f734505fa2f79fe653c7/Lib/threading.py#L1115 [^3]: mpage@8194653 --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <[email protected]>

mpage added the type-feature A feature request or enhancement label Jan 18, 2024

Eclips4 added the topic-free-threading label Jan 19, 2024

colesbury assigned mpage Jan 19, 2024

colesbury mentioned this issue Jan 19, 2024

PEP 703 -- Making the Global Interpreter Lock Optional in CPython #108219

Open

bedevere-app bot mentioned this issue Feb 1, 2024

gh-114271: Fix race in Thread.join() #114839

Merged

mpage added a commit to mpage/cpython that referenced this issue Feb 5, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

19d7af1

ericsnowcurrently pushed a commit that referenced this issue Feb 12, 2024

gh-114271: Make PyInterpreterState.threads.count thread-safe in fre…

de7d67b

…e-threaded builds (gh-115093) Use atomics to mutate PyInterpreterState.threads.count.

fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this issue Feb 14, 2024

pythongh-114271: Make PyInterpreterState.threads.count thread-safe …

c62acde

…in free-threaded builds (pythongh-115093) Use atomics to mutate PyInterpreterState.threads.count.

mpage added a commit to mpage/cpython that referenced this issue Feb 16, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

c86d349

mpage added a commit to mpage/cpython that referenced this issue Feb 16, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

40e4b36

mpage added a commit to mpage/cpython that referenced this issue Feb 16, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

4ed1083

mpage added a commit to mpage/cpython that referenced this issue Feb 16, 2024

Merge branch 'main' into pythongh-114271-_thread-ThreadHandle

9ec6d23

mpage added a commit to mpage/cpython that referenced this issue Feb 16, 2024

Merge branch 'pythongh-114271-_thread-ThreadHandle' into pythongh-11427…

012ea77

…-cherry-pick-ThreadHandle

mpage added a commit to mpage/cpython that referenced this issue Mar 5, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

34b6065

bedevere-app bot mentioned this issue Mar 6, 2024

gh-114271: Make _thread.lock thread-safe in free-threaded builds #116433

Merged

mpage added a commit to mpage/cpython that referenced this issue Mar 8, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

57e106d

colesbury added a commit to colesbury/cpython that referenced this issue Mar 11, 2024

Merge branch 'pythongh-114271-remove-tstate_lock' into nogil-integration

b45c98f

mpage added a commit to mpage/cpython that referenced this issue Mar 11, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

ccd1c2e

colesbury added a commit to colesbury/cpython that referenced this issue Mar 12, 2024

Merge branch 'pythongh-114271-remove-tstate_lock' into nogil-integration

7678bcf

swtaarrs mentioned this issue Mar 13, 2024

Audit all built-in modules for thread safety #116738

Open

colesbury added a commit to colesbury/cpython that referenced this issue Mar 13, 2024

Merge branch 'pythongh-114271-remove-tstate_lock' into nogil-integration

e4fb11c

colesbury added a commit to colesbury/cpython that referenced this issue Mar 13, 2024

Merge branch 'pythongh-114271-remove-tstate_lock' into nogil-integration

0cd9f9c

bedevere-app bot mentioned this issue Mar 14, 2024

gh-114271: Clean Up _threadmodule.c #116776

Closed

mpage added a commit to mpage/cpython that referenced this issue Mar 15, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

ee96259

colesbury added a commit to colesbury/cpython that referenced this issue Mar 15, 2024

Merge branch 'pythongh-114271-remove-tstate_lock' into nogil-integration

c37a0d0

pitrou added a commit to mpage/cpython that referenced this issue Mar 16, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

9a8ea9b

pitrou added a commit to mpage/cpython that referenced this issue Mar 16, 2024

Merge branch 'main' into pythongh-114271-remove-tstate_lock

339b2e6

mpage closed this as completed Apr 10, 2024

bsteffensmeier mentioned this issue Aug 26, 2024

Update threading checks to be compatible with Python 3.13 ninia/jep#551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `_threadmodule.c` thread-safe in `--disable-gil` builds #114271

Make `_threadmodule.c` thread-safe in `--disable-gil` builds #114271

mpage commented Jan 18, 2024 •

edited by bedevere-app bot

Loading

mpage commented Jan 19, 2024

colesbury commented Apr 10, 2024

mpage commented Apr 10, 2024 •

edited

Loading

Make _threadmodule.c thread-safe in --disable-gil builds #114271

Make _threadmodule.c thread-safe in --disable-gil builds #114271

Comments

mpage commented Jan 18, 2024 • edited by bedevere-app bot Loading

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

mpage commented Jan 19, 2024

colesbury commented Apr 10, 2024

mpage commented Apr 10, 2024 • edited Loading

Make `_threadmodule.c` thread-safe in `--disable-gil` builds #114271

Make `_threadmodule.c` thread-safe in `--disable-gil` builds #114271

mpage commented Jan 18, 2024 •

edited by bedevere-app bot

Loading

mpage commented Apr 10, 2024 •

edited

Loading