Repository navigation
Conversation
|
CC @ngoldbaum if you have bandwdith, thanks! |
|
I think this needs a different approach. First, this doesn’t protect all accesses to the handler pointers, so it’s incomplete. It also doesn’t protect all the state the GIL previously serialized access to, so it’s incomplete from that perspective. Finally, there are no tests and fixes really should come with tests that run quickly and are deterministically broken before the fixes are applied. Second, a mutex cannot protect every read of signal state. PyErr_SetInterruptEx() may run inside a C signal handler, where acquiring a mutex is unsafe. Correctly implementing signal handlers and handling signal safety is incredibly difficult, see https://iafisher.com/2026/08/sigprint and other posts in that series for examples. That means we’d need to come up with some other signal-safe approach anyway. A lock-free design could cover the path that must be signal-safe as well as reads that need the handler object. Here is an outline of a design: signal.signal() could update an atomic flag indicating whether a Python handler is installed. PyErr_SetInterruptEx() could read that flag without touching a Python object. For signal.getsignal() and Python signal dispatch, each reader could mark a read in progress before loading an atomic handler pointer and taking a strong reference. After replacing the pointer, signal.signal() could keep the old handler alive until earlier reads finish. You’d need to add bookkeeping to track live reads and add or expand tests for the interactions between threads and signals to stress-test the bookkeeping. |
This addresses one of the found race conditions visible with TSAN and pydebug for #116738 in
Modules/signalmodule.c.Was not able to see this crashing on regular build, unsure what the precedence here is in terms off addressing this likely low issue race for FT but it came up as a potential gap in the linked issue in this module.