Message 103214 - Python tracker (original) (raw)
Will the modified test fail on platforms that don't define HAVE_SIGACTION?
Only if they also have siginterrupt, which seems unlikely (as neologix explained). The implemented behavior on such platforms is unmodified from current trunk, while the test requires new behavior.
I think it's probably worth testing this new behavior separately from the test for the existing behavior anyway, for the sake of clarity. If it turns out there's a platform with siginterrupt but not sigaction, then that'll also let the test be skipped there, which is nice (though this case seems unlikely to come up).
I'm not exactly sure how we will know if it is expected to fail, though. I don't think HAVE_SIGACTION is exposed nicely to Python right now. Perhaps the signal module should make this information available (it's somewhat important now, as it will mean the difference between a nicely working signal.siginterrupt and a not-so-nicely working one).
This quirk probably also bears a mention in the siginterrupt documentation.
A few other thoughts on the code nearby:
There seems to be no good reason to special case SIGCHLD in signal_handler. The comment about infinite recursion has no obvious interpretation to me. Fortunately, this is irrelevant on platforms with sigaction, because the handler remains installed anyway!
The distance between the new #ifndef HAVE_SIGACTION check and the corresponding #ifdef HAVE_SIGACTION in pythonrun.c is somewhat unfortunate. Someone could very easily change the logic in PyOS_setsig and disturb this relationship. I'm not sure how this could best be fixed. A general idea which presents itself is that both of these pieces of code could use a new identifier to make this choice, and that identifier could be named/documented such that it is obvious that it is used in two different places which must be kept synchronized. Or, some more comments in both places might help. It seems likely that the remaining SIGCHLD check in handle_signal is only there because whoever updated PyOS_setsig to use sigaction didn't know about it/remember it.
Aside from those points, the fix seems correct and good.