gh-126907: Lock the atexit state on the free-threaded build

[ZeroIntensity]

• Issue: Running atexit from threads in free-threading build segfaults #126907

[picnixz]

Quick review while waiting for my train

[vstinner]

Overall, IMO the approach is valid. Here is a first review.

[vstinner]

LGTM.

@colesbury @mpage: Do you want want to review this change?

[kumaraditya303]

I think it's easy to introduce reentrancy issues in this code, how about using a critical section instead of mutex?

[colesbury]

Thanks for taking this on Peter. Like @kumaraditya303 suggested, I think using critical sections will simplify the code and avoid some re-entrancy issues. Having to explicitly unlock the mutex inside some functions is a sign that the code should be refactored:

The low-level callbacks (ll_callbacks) and high-level Python callbacks are pretty different:

• The PyMutex for modifying ll_callbacks is fine
• Use a critical section for protecting state->callbacks. Ideally, the acquisition happens at the module boundaries. Use argument clinic and @critical_section for the module methods.
• I suspect a lot of the code would be simplified if state->callbacks were a Python list object

[colesbury]

The explicit unlocking means that this still has re-entrancy and concurrency issues if the callback list is modified during the comparison. callbacks[i] may be a different callback.

Using critical sections doesn't completely avoid the problem if __eq__ is particularly weird, but it avoids it for the common case of identity comparison and the issues are limited to the cases where the GIL-enabled build has issues as well.

[ZeroIntensity]

Ok, I'll switch this over to critical sections. For now, I'd rather just ignore ll_callbacks until I fix PyUnstable_AtExit (gh-127793).

I suspect a lot of the code would be simplified if state->callbacks were a Python list object

It could, but it also might be a PITA because we'd need to introduce capsules into the mix. I'll play around with it and see how it looks.

[colesbury]

It could, but it also might be a PITA because we'd need to introduce capsules into the mix. I'll play around with it and see how it looks.

I don't think you need capsules, which I agree would be a pain. I think you can use a tuple of three items instead of atexit_py_callback and replace atexit_py_callback **callbacks with a list of those tuples.

Most of what atexit does with the Python callbacks is essentially list manipulation and we already have thread-safe implementations for PyListObject. You might need to expose list_remove as an internal-only function though.

The Python callbacks are executed while other threads are still running (and can possibly modify the callbacks). Using a PyListObject makes it easier to make a local copy of the callbacks before iterating over them, which avoids some re-entrancy and thread-safety issues in atexit_callfuncs that aren't limited to the free threading build.

[colesbury]

To be clear, I am only suggesting a PyListObject instead of atexit_py_callback **callbacks (the high-level Python callbacks) and not instead of atexit_callback *ll_callbacks (the low-level C callbacks).

[ZeroIntensity]

I was just thinking about using a list because it has a nicer interface, not for thread safety.

The Python callbacks are executed while other threads are still running (and can possibly modify the callbacks).

They are? That's not good. The two places that execute atexit callbacks (apart from manually doing it) are

cpython/Python/pylifecycle.c

Line 2040 in 5121685

_PyAtExit_Call(tstate->interp);

and

cpython/Python/pylifecycle.c

Line 2440 in 5121685

_PyAtExit_Call(tstate->interp);

Both of which are supposed to happen when no threads are left. I guess daemon and C threads could screw things up, but I'm willing to write that off as a "play stupid games, win stupid prizes."

[ZeroIntensity]

Marking as DO-NOT-MERGE while I refactor this. I'll (hopefully) have this ready by the end of the week.

[vstinner]

You might create a new PR if you switch to critical sections, so we can compare the two approaches, and the PR history is easier to follow.

[ZeroIntensity]

Closing now that #127935 got merged. Thanks to everyone that reviewed!