[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.