[Python-ideas] Importance of noticing new signals

[Antoine Pitrou]

On Fri, 3 Nov 2017 17:22:07 +0200
Koos Zevenhoven <k7hoven at 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.