[WIP] bpo-30703: More reentrant signal handler

[vstinner]

Modify the signal handler to not call Py_AddPendingCall() function
since this function uses a lock and a list, and so is unlikely to be
reentrant. Add a new _PyEval_SignalReceived() function which only
writes into an atomic variable and so is reentrant.

[pitrou]

s/use/uses/

[pitrou]

Also say "signal-safe" rather than "reentrant", since otherwise it's not clear how Py_AddPendingCall can be called reentrantly.

[pitrou]

You can remove "UNIX", as even Windows has a couple of signals :-)

[pitrou]

What about the part without WITH_THREAD? :-)

[pitrou]

Perhaps this should be called after the check for main_thread and busy?

[pitrou]

Ok, I think there is a problem with this patch.

What if a C signal is received just after PyErr_CheckSignals()? SIGNAL_PENDING_CALLS() will simply be called for the signal to be examined next. However, after that, Py_MakePendingCalls will continue executing and will eventually call UNSIGNAL_PENDING_CALLS() as it will think there are no more pending calls to execute. Thus the new signal will fail to wake up the eval loop again.

[pitrou]

This is why, by the way, Py_AddPendingCall calls SIGNAL_PENDING_CALLS() with the pending_lock taken.

[pitrou]

A possible solution should be to call UNSIGNAL_PENDING_CALLS() before calling PyErr_CheckSignals(), and remove the UNSIGNAL_PENDING_CALLS() call from the Py_MakePendingCalls() loop.

Also, perhaps change the bit that calls Py_MakePendingCalls() in the eval loop to:

            while (_Py_atomic_load_relaxed(&pendingcalls_to_do)) {
                if (Py_MakePendingCalls() < 0)
                    goto error;
            }

(i.e. change the if to a while)

[pitrou]

It's difficult to reproduce the race condition described above but here is a script that may work:

import os
import signal
import time

sigs = []

def handler1(signum, frame):
    os.kill(os.getpid(), signal.SIGPROF)

def handler2(signum, frame):
    signal.setitimer(signal.ITIMER_REAL, 1e-6)

def handler_itimer_real(signum=None, frame=None):
    sigs.append(signum)


if __name__ == "__main__":
    N = 10
    signal.signal(signal.SIGIO, handler1)
    signal.signal(signal.SIGPROF, handler2)
    signal.signal(signal.SIGALRM, handler_itimer_real)
    for i in range(N):
        os.kill(os.getpid(), signal.SIGIO)
        for j in range(3):
            time.sleep(1e-3)

    print("sigs =", len(sigs), sigs)

[pitrou]

This patch applied to your PR should make things better:
https://gist.github.com/pitrou/e282723f047fc5260ff9c2e840a3b87c

[vstinner]

This patch applied to your PR should make things better: https://gist.github.com/pitrou/e282723f047fc5260ff9c2e840a3b87c

Please push directly into my branch, you are allowed to do that ;-) See maybe https://docs.python.org/devguide/gitbootcamp.html#editing-a-pull-request-prior-to-merging

[vstinner]

I added [WIP] to the title, since I didn't test my change. I was more to discuss a practical solution to the problem. It seems like you spotted bugs in my implementation, thanks ;-)

[pitrou]

That doesn't work. I get:

(haypo-signal_signal)$ git push --set-upstream haypo 
fatal: remote error: 
  You can't push to git://github.com/haypo/cpython.git
  Use https://github.com/haypo/cpython.git
(haypo-signal_signal)$ git push --set-upstream https://github.com/haypo/cpython.git
Username for 'https://github.com': ^C

[pitrou]

I'll create another PR instead of dealing with git cruft.

[pitrou]

See #2415

[vstinner]

That doesn't work. I get: (...)

It seems like you used the HTTPS URL. I suggest you to use the SSH URL.

Moreover, when I try to push to a different repository, I try to only push a single branch. For example, to push the to BRANCH branch of REMOTE remote, you can type:

git push REMOTE HEAD:BRANCH -f

HEAD uses the current branch, ":BRANCH" means that you push to REMOTE:BRANCH. Non obvious syntax, but I like it.

[pitrou]

I suggest you to use the SSH URL.

It is what I tried before (see above) :-)

(haypo-signal_signal)$ git push --set-upstream haypo 
fatal: remote error: 
  You can't push to git://github.com/haypo/cpython.git
  Use https://github.com/haypo/cpython.git

git push REMOTE HEAD:BRANCH -f

I'm almost sure I'll have forgotten that the next time I'll need it :-/

[vstinner]

@pitrou completed and polished my PR in his PR #2415 (commit c08177a) which has been merged, so I abandon this PR.