Repository navigation
Conversation
Documentation build overview
450 files changed ·
|
|
@cobed95 Sorry for the delay, I will take a look at this week. |
|
Yeah I will do this during this sprint |
| // lets two threads select the same slot and return the same ID. | ||
| // Losing the race means the slot is taken, so just try the next one. | ||
| PyContext_WatchCallback expected = NULL; | ||
| if (_Py_atomic_compare_exchange_ptr(&interp->context_watchers[i], |
There was a problem hiding this comment.
It doesn not gurentee to set set_context_watcher_bit if CAS is not success, you may run loop here.
There was a problem hiding this comment.
You’re right. The CAS only made claiming the slot atomic; setting the bit was still a separate step, so the add as a whole wasn’t. (It’s a strong CAS, so the claim itself didn’t need a retry loop.) I’ve dropped it: PyContext_AddWatcher() now does a plain check and store inside a critical section.
| clear_context_watcher_bit(interp, watcher_id); | ||
| FT_ATOMIC_STORE_PTR_RELEASE(interp->context_watchers[watcher_id], NULL); |
There was a problem hiding this comment.
This will make issue if one thread clear watcher_id and other thread try to add watcher_id.
Seqeuntial operation should be gurenteed.
There was a problem hiding this comment.
Agreed, and I could reproduce it. With the threads lined up, two concurrent PyContext_ClearWatcher() calls on the same ID both returned 0, and on current main sometimes three or four do. Add and Clear are now serialized by a critical section on a new per-interpreter mutex. Notification still doesn’t take it.
I added test_concurrent_clear_same_watcher for this. It fails every time on main and passes now.
| // Stop notification selecting this slot before retiring the callback, so | ||
| // the window in which a notifier sees the bit but loads NULL is as short | ||
| // as possible. That window is benign: notify_context_watchers() skips a | ||
| // NULL callback. |
There was a problem hiding this comment.
Please rewrite comment as concise as possible.
There was a problem hiding this comment.
Done, it’s one line now:
// notify_context_watchers() skips a NULL callback.
corona10
left a comment
corona10
left a comment
There was a problem hiding this comment.
I thint that following functions should use critical section to gurentee sequetiallly consistant output.
- PyContext_ClearWatcher
- PyContext_AddWatcher
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Hi everyone, I will work on this by the end of this weekend. Thanks for your patience. |
Resolve the conflict with pythongh-159739 in notify_context_watchers(): keep the acquire load, and cover both ways the callback can be NULL in the comment.
Each step of the lock-free version was atomic, but an add or clear as a whole was not: two concurrent PyContext_ClearWatcher() calls on one ID could both succeed, and a clear landing in the middle of an add could leave the active bit set over an empty slot. Run both under a critical section on a new per-interpreter mutex and drop the CAS. Notification still does not take the lock. Add test_concurrent_clear_same_watcher, which fails on the previous revision, and drop a test docstring's reference to the assert that pythongh-159739 removed from main. Update NEWS to match.
|
Thanks for the review. Every access in the previous revision was atomic, so this was a race TSan couldn’t have flagged.
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @corona10: please review the changes made to this pull request. |
The test aborted its barrier in a finally block, so on success it could break the last round while the workers were still waking from it. They then raised BrokenBarrierError, which regrtest reports as an uncaught thread exception, failing the free-threaded CI jobs under --fail-env-changed. Abort only when the main loop fails, and let the workers return if the barrier is broken.
| if sys._is_gil_enabled(): | ||
| # The calls cannot overlap, and the busy-waits would just | ||
| # contend for the GIL. | ||
| self.skipTest("needs the GIL to be disabled") |
There was a problem hiding this comment.
Are all tests in this module already skip with the GIL enabled? There is a already https://github.com/python/cpython/blob/main/Lib/test/test_free_threading/__init__.py#L7
| #ifdef Py_GIL_DISABLED | ||
| (void)_Py_atomic_and_uint8(&interp->active_context_watchers, (uint8_t)~bit); | ||
| #else | ||
| interp->active_context_watchers &= (uint8_t)~bit; | ||
| #endif |
There was a problem hiding this comment.
should this be added as a new FT_ATOMIC_ wrapper?
| except RuntimeError: | ||
| continue # all CONTEXT_MAX_WATCHERS slots taken |
There was a problem hiding this comment.
Is there a reason to catch runtime errors which could mask test errors such as using too many slots?
#155619
Reproduction
When threads race, the watcher registry hands out duplicate IDs and accepts duplicate clears. On current main the new tests fail on a free-threaded debug build:
test_concurrent_add_clear_watchersValueError: No context watcher set for ID 0test_concurrent_clear_same_watchertest_clear_watcher_races_notificationValueErrorThe same binary with
PYTHON_GIL=1passes. (The double-clear test skips itself there, since the calls can't overlap.)gh-159739 already replaced the
assert(cb != NULL)innotify_context_watchers()with a NULL check, so a clear racing with a context switch no longer crashes. What's left is that the registry is read and written with plain accesses from several threads, and an add or clear isn't atomic as a whole.Fix
PyContext_AddWatcher()andPyContext_ClearWatcher()now run underPy_BEGIN_CRITICAL_SECTION_MUTEX()on a newPyMutexin the interpreter state, the same pattern asTYPE_LOCK. They run about once per extension, so the lock costs nothing that matters.notify_context_watchers()runs on every context switch and still takes no lock. It loads each callback with an acquire load, paired with the release store inPyContext_AddWatcher(), and keeps gh-159739's NULL check for a watcher cleared by an earlier callback or by another thread. I kept the bitmask rather than scanning all eight slots because it makes the common zero-watcher case one load and a not-taken branch.Docs
Doc/c-api/contextvars.rsthad no threading guidance at all, unlike dict and type watchers, so I added some.Testing
The new tests pass 20/20 runs.
test_free_threading,test_context,test_capi,test_contextlibandtest_asynciopass on a free-threaded debug build (5,149 tests), and so does the same binary withPYTHON_GIL=1(1,690 tests).test_concurrent_clear_same_watcherreleases its threads with a busy-wait instead of aBarrier. A barrier wakes them too far apart to overlap and found nothing in 20,000 rounds, while the busy-wait hits the race in about a quarter of rounds.No ThreadSanitizer run from me. It wouldn't catch the double clear anyway: in the previous revision every access was already atomic, so there was no data race to report, just clears that weren't atomic as a whole.
Benchmarks
pyperformance
async_tree,async_tree_io,async_generators(asyncio runs each task step throughctx.run()), plusnbodyas a control that never enters a context. Free-threaded build, baseline vs. this change:These were measured on the previous revision. Notification, the only path they exercise, hasn't changed since; add and clear now take a lock, but they aren't on that path.
The default build was the same story, except
async_generatorscame out 1.08x slower there. I think that one is noise: its spread is about 12% of the mean, t=-2.54, and the same benchmark went faster on the free-threaded build. These were plain-O3builds with--faston a laptop, so they answer "does this cost anything" and not much else; happy to redo them properly if it matters.