On Fri, 3 Nov 2017 17:22:07 +0200 Koos Zevenhoven <k7hoven@gmail.com> wrote:
Maybe you are concerned about whether some nuances and recent changes to signal handling could lead to harmful change in behavior in some meaningful edge cases? I can at least say that my PyErr_PROBE_SIGNALS() proposal does not introduce such issues, if the difference is documented properly:
"""PyErr_PROBE_SIGNALS() is meant for performance-critical code and is not 100% guaranteed to always see the most recent signals. If a signal being deferred is a concern, use PyErr_CheckSignals() instead."""
Agreed.
But more generally, if we could assume that trip_signal() and PyErr_CheckSignals() always happen in the same "CPU thread", then we wouldn't need pyatomic.h here at all. The fact that the code currently assumes that all Python signal handlers should run in the same Python thread takes care of some of these concerns without needing locks etc.
Hmm... *Python* signal handlers (registered with signal.signal()) all run in the same thread, but we have no way to ensure to ensure that trip_signal() (which is a C-level signal handler called by the OS) will run in the same thread. Even if we were to mask all signals in non-main threads started by Python (e.g. with threading.Thread), there can still be threads created by third-party C libraries that we don't have any control over.
Some other concerns I can imagine by looking at some of the code in Modules/signalmodule.c:
(1) If trip_signal() and PyErr_CheckSignals() are executed concurrently, trip_signal() might set a new signal flag (using relaxed memory order) while PyErr_CheckSignals is still running. Then if PyErr_CheckSignals() sets is_tripped to zero *after* trip_signal() sets it to 1, then the new signal might be deferred until the next time *some* new signal arrives, which could take an arbitrarily long amount of time, I suppose.
However, it looks like this problem has been solved by always setting is_tripped to zero (with strict SEQ_CST memory order) *before* handling the individual signals. So if trip_signal() has already set is_tripped to 1 (with SEQ_CST), that prevents PyErr_CheckSignals from setting is_tripped to zero (with SEQ_CST) *and not* handling the signal. If trip_signal() has not yet finished, and therefore not set is_tripped to 1 yet, it will cause the next call to PyErr_CheckSignals to catch the signal.
(2) Again, if trip_signal() and PyErr_CheckSignals() execute concurrently, it might happen that PyErr_CheckSignals() handles the signal *before* trip_signal sets is_tripped to 1. That would cause the next call to PyErr_CheckSignals() to think there's an unhandled signal, but will most likely not find one, because it was already handled on the previous call. But that just effectively means that nothing is done. In fact, there's a comment in the code that mentions this.
(3, 4, ...) Of course there's more to take care of there, but that's unrelated to my PyErr_PROBE_SIGNALS() proposal. Anyway, at least (1) and (2) seem to already have been taken care of, and I assume you are aware of that.
Yes, that was my analysis too (and why I implemented it this way in the first place). But I'm glad you confirm it, since on such topics it's always easy to miss a potential problem. Regards Antoine.