Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Closed
vstinner wants to merge 1 commit intopython:masterpython/cpython:masterfrom
vstinner:signal_signalCopy head branch name to clipboard
Closed

[WIP] bpo-30703: More reentrant signal handler#2408
vstinner wants to merge 1 commit intopython:masterpython/cpython:masterfrom
vstinner:signal_signalCopy head branch name to clipboard

Conversation

@vstinner
Copy link
Member

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.

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.
{
/* 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 (use a lock and a list). */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/use/uses/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

int r = 0;

/* Python signal handler doesn't really queue a callback: it only signals
that an UNIX signal was received, see _PyEval_SignalReceived(). */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitrou
Copy link
Member

pitrou commented Jun 26, 2017

What about the part without WITH_THREAD? :-)


/* Python signal handler doesn't really queue a callback: it only signals
that an UNIX signal was received, see _PyEval_SignalReceived(). */
if (PyErr_CheckSignals() < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pitrou pitrou added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels Jun 26, 2017
@pitrou
Copy link
Member

pitrou commented Jun 26, 2017

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
Copy link
Member

pitrou commented Jun 26, 2017

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

@pitrou
Copy link
Member

pitrou commented Jun 26, 2017

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
Copy link
Member

pitrou commented Jun 26, 2017

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
Copy link
Member

pitrou commented Jun 26, 2017

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

@vstinner
Copy link
Member Author

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 vstinner changed the title bpo-30703: More reentrant signal handler [WIP] bpo-30703: More reentrant signal handler Jun 26, 2017
@vstinner
Copy link
Member Author

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
Copy link
Member

pitrou commented Jun 26, 2017

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
Copy link
Member

pitrou commented Jun 26, 2017

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

@pitrou
Copy link
Member

pitrou commented Jun 26, 2017

See #2415

@vstinner
Copy link
Member Author

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
Copy link
Member

pitrou commented Jun 26, 2017

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
Copy link
Member Author

vstinner commented Jul 5, 2017

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

@vstinner vstinner closed this Jul 5, 2017
@vstinner vstinner deleted the signal_signal branch July 5, 2017 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.