bpo-30703: Improve signal delivery #2415

pitrou · 2017-06-26T17:18:51Z

Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions.

mention-bot · 2017-06-26T17:18:57Z

@pitrou, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @1st1 to be potential reviewers.

pitrou · 2017-06-26T21:54:44Z

The test I'm adding here fails without the rest of the patch (on Linux and OS X). This means our signal delivery logic really had holes in it.

pitrou · 2017-06-27T09:16:46Z

@tim-one, would you like to review this one?

…l_delivery

vstinner

Since I wrote the first version of this change, I like the design, haha. More seriously, the overall change LGTM.

But I added a few comments with some questions.

vstinner · 2017-06-28T20:20:21Z

Modules/signalmodule.c

@@ -1556,8 +1552,10 @@ PyErr_CheckSignals(void)
                                           arglist);
                Py_DECREF(arglist);
            }
-            if (!result)
+            if (!result) {
+                is_tripped = 1;


IHMO it would be more explicit to have a "error:" label and use goto here, and if arglist creation failed. But well, it's just a coding style suggestion, it's up to you ;-)

vstinner · 2017-06-28T20:21:33Z

Python/ceval.c

+{
+    /* bpo-30703: Function called when the C signal handler of Python gets a
+       signal. We cannot queue a callback using Py_AddPendingCall() since this
+       function is not reentrant (uses a lock and a list). */


you asked me to write "async-signal safe" :-) maybe remove "(uses a lock and a list)"

Oh, thank you. I had forgotten that.

vstinner · 2017-06-28T20:23:34Z

Python/ceval.c

@@ -408,6 +415,16 @@ Py_MakePendingCalls(void)
    if (busy)
        return 0;
    busy = 1;


I don't understand well the purpose of busy... is it a flag to catch reentrant call? If no, maybe it should use volalite? If yes, please add a comment on the variable declaration to document this flag.

"don't perform recursive pending calls" as written above :-)

vstinner · 2017-06-28T20:24:22Z

Python/ceval.c

+       that a signal was received, see _PyEval_SignalReceived(). */
+    UNSIGNAL_PENDING_CALLS();
+    if (PyErr_CheckSignals() < 0) {
+        busy = 0;


I suggest to move this code at the end of the function, in a new "error:" label and use goto. IMHO it's more readable like that (set busy to 1 at the function entry and set it to 0 at exit) (and it would avoid code duplication).

ok, will do.

vstinner · 2017-06-28T20:27:31Z

Python/ceval.c

@@ -408,6 +415,16 @@ Py_MakePendingCalls(void)
    if (busy)
        return 0;


Would you mind to add assert(PyGILState_Check()); at the function entry to make it more explicit that this function requires to hold the GIL?

(Right now, it's not obvious for me if the pending_lock initialization is safe.)

Ok, will do.

vstinner · 2017-06-28T20:28:11Z

Python/ceval.c

    UNSIGNAL_PENDING_CALLS();
+    if (PyErr_CheckSignals() < 0) {


Please copy the comment before the if:

/* Python signal handler doesn't really queue a callback: it only signals

that a signal was received, see _PyEval_SignalReceived(). */

vstinner · 2017-06-28T20:30:00Z

Lib/test/test_signal.py

@@ -941,6 +942,101 @@ def handler(signum, frame):
                                (exitcode, stdout))


+class StressTest(unittest.TestCase):


Oh... I dislike such tests, I expect that I will have to fix corner cases in the test itself on BSD, Solaris, etc. I understand why you wrote the test, but I also expect that it will be a pain to "maintain" it :-(

vstinner · 2017-06-28T20:31:20Z

Lib/test/test_signal.py

+        setsig(signal.SIGALRM, second_handler)  # for ITIMER_REAL
+
+        expected_sigs = 0
+        deadline = time.time() + 15.0


10 000 and 15 seconds? Oh, this test seems expensive :-/ Maybe tag it using the "cpu" resource? Or reduce these parameters?

The test takes 0.5 second on my machine. The large 15 seconds timeout was just there to make you (and the buildbots) happy :-)

vstinner · 2017-06-28T20:32:39Z

About the test, I would even prefer to not have such test. Or maybe only run it a few times (ex: 100 signals, not 10 000).

vstinner

LGTM. I'm not confortable to backport this change on stable versions right now. If you want to backport it, maybe wait at least one week to see if the test works well on all buildbots.

pitrou · 2017-06-28T21:22:01Z

Agreed. If it goes well then I may consider a backport to 3.6. I think 2.7 and 3.5 are out of question.

pitrou · 2017-06-28T22:01:54Z

It seems some systems may have crappy resolution for setitimer. For example, the FreeBSD man page says:

Time values smaller than the resolution of the system clock are rounded up to this resolution (typically 10 milliseconds).

(https://www.freebsd.org/cgi/man.cgi?query=setitimer&apropos=0&sektion=2&manpath=FreeBSD+10.0-RELEASE&arch=default&format=html)

This might explain the failure here: http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.x%203.x/builds/275/steps/test/logs/stdio
(note signal 14 is SIGALRM)

pitrou · 2017-06-28T22:03:42Z

Same for AIX:

If a nonprivileged user attempts to submit a fine granularity timer (that is, a timer request of less than 10 milliseconds), the timer request interval is automatically raised to 10 milliseconds.

(https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.basetrf1/getinterval.htm)

vstinner · 2017-06-28T23:10:56Z

I told you. Your test looks like a nightmare to me :-) In the worst case, just make it specific to Linux?

pitrou · 2017-06-28T23:11:58Z

Please stop being offensive with my test :-)

vstinner · 2017-06-28T23:13:16Z

Ok, as soon as you stop to harass my little buildbots!

@Haypo

* Improve signal delivery Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions. * Remove unused function * Improve comments * Add stress test * Adapt for --without-threads * Add second stress test * Add NEWS blurb * Address comments @Haypo. (cherry picked from commit c08177a)

@Haypo

* [3.6] bpo-30703: Improve signal delivery (GH-2415) * Improve signal delivery Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions. * Remove unused function * Improve comments * Add stress test * Adapt for --without-threads * Add second stress test * Add NEWS blurb * Address comments @Haypo. (cherry picked from commit c08177a) * bpo-30796: Fix failures in signal delivery stress test (#2488) * bpo-30796: Fix failures in signal delivery stress test setitimer() can have a poor minimum resolution on some machines, this would make the test reach its deadline (and a stray signal could then kill a subsequent test). * Make sure to clear the itimer after the test

Improve signal delivery

0a68ec4

Avoid using Py_AddPendingCall from signal handler, to avoid calling signal-unsafe functions.

the-knights-who-say-ni added the CLA signed label Jun 26, 2017

pitrou mentioned this pull request Jun 26, 2017

[WIP] bpo-30703: More reentrant signal handler #2408

Closed

pitrou changed the title ~~Improve signal delivery~~ bpo-30703: Improve signal delivery Jun 26, 2017

pitrou added 2 commits June 26, 2017 20:00

Remove unused function

161cd38

Improve comments

b577db5

pitrou added the type-bug An unexpected behavior, bug, or error label Jun 26, 2017

pitrou mentioned this pull request Jun 26, 2017

bpo-30808: Use _Py_atomic API for concurrency-sensitive signal state #2417

Merged

pitrou added 2 commits June 26, 2017 22:34

Merge branch 'master' into signal_delivery

e0ce64b

Add stress test

e9cec43

pitrou added 4 commits June 27, 2017 11:36

Adapt for --without-threads

ea34f8c

Add second stress test

542465c

Merge branch 'master' of https://github.com/python/cpython into signa…

a2851eb

…l_delivery

Add NEWS blurb

a71a8fe

vstinner reviewed Jun 28, 2017

View reviewed changes

Address comments @Haypo

c37e5d8

vstinner approved these changes Jun 28, 2017

View reviewed changes

pitrou merged commit c08177a into python:master Jun 28, 2017

pitrou deleted the signal_delivery branch June 28, 2017 21:29

ma8ma mentioned this pull request Jul 5, 2017

bpo-30854: Fix compile error when --without-threads #2581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30703: Improve signal delivery #2415

bpo-30703: Improve signal delivery #2415

pitrou commented Jun 26, 2017

mention-bot commented Jun 26, 2017

pitrou commented Jun 26, 2017

pitrou commented Jun 27, 2017

vstinner left a comment

vstinner Jun 28, 2017

vstinner Jun 28, 2017

pitrou Jun 28, 2017

vstinner Jun 28, 2017

pitrou Jun 28, 2017

vstinner Jun 28, 2017

pitrou Jun 28, 2017

vstinner Jun 28, 2017

pitrou Jun 28, 2017

vstinner Jun 28, 2017

vstinner Jun 28, 2017

vstinner Jun 28, 2017

pitrou Jun 28, 2017

vstinner commented Jun 28, 2017

vstinner left a comment

pitrou commented Jun 28, 2017

pitrou commented Jun 28, 2017

pitrou commented Jun 28, 2017

vstinner commented Jun 28, 2017 via email

pitrou commented Jun 28, 2017

vstinner commented Jun 28, 2017 via email

		@@ -408,6 +415,16 @@ Py_MakePendingCalls(void)
		if (busy)
		return 0;

		@@ -941,6 +942,101 @@ def handler(signum, frame):
		(exitcode, stdout))


		class StressTest(unittest.TestCase):

bpo-30703: Improve signal delivery #2415

bpo-30703: Improve signal delivery #2415

Conversation

pitrou commented Jun 26, 2017

mention-bot commented Jun 26, 2017

pitrou commented Jun 26, 2017

pitrou commented Jun 27, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Jun 28, 2017

vstinner left a comment

Choose a reason for hiding this comment

pitrou commented Jun 28, 2017

pitrou commented Jun 28, 2017

pitrou commented Jun 28, 2017

vstinner commented Jun 28, 2017 via email

pitrou commented Jun 28, 2017

vstinner commented Jun 28, 2017 via email