On Fri, 3 Nov 2017 17:22:07 +0200 Koos Zevenhoven <k7ho...@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. _______________________________________________ Python-ideas mailing list Python-ideas@python.org https://mail.python.org/mailman/listinfo/python-ideas Code of Conduct: http://python.org/psf/codeofconduct/
