Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-30039: Don't run signal handlers while resuming a yield from stack #1081

Merged
merged 4 commits into from May 17, 2017

Conversation

Projects
None yet
9 participants
@njsmith
Copy link
Contributor

commented Apr 11, 2017

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

  • call send() on the outermost generator
  • this enters _PyEval_EvalFrameDefault, which re-executes the
    YIELD_FROM opcode
  • which calls send() on the next generator
  • which enters _PyEval_EvalFrameDefault, which re-executes the
    YIELD_FROM opcode
  • ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception instead of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all.

The included test fails before this patch, but passes afterwards.

@mention-bot

This comment has been minimized.

Copy link

commented Apr 11, 2017

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

@njsmith

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2017

CC @1st1

AFAIK this should be good to go except for missing a NEWS entry (and fingers crossed on the CI bots...). I didn't do that because it's, y'know, a mess for conflicts and I'm not sure what the current status of that discussion is.

@1st1

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

LGTM.

(for those few interested in why this is needed you can read Nathaniel's insightful blog post here: https://vorpus.org/blog/control-c-handling-in-python-and-trio/#twisted, in addition to the issue/PR)

@1st1 1st1 self-requested a review Apr 20, 2017

@1st1

1st1 approved these changes Apr 20, 2017

@@ -1064,9 +1064,11 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
Py_MakePendingCalls() above. */

if (_Py_atomic_load_relaxed(&eval_breaker)) {
if (_Py_OPCODE(*next_instr) == SETUP_FINALLY) {
if (_Py_OPCODE(*next_instr) == SETUP_FINALLY
|| _Py_OPCODE(*next_instr) == YIELD_FROM) {

This comment has been minimized.

Copy link
@1st1

1st1 Apr 20, 2017

Member

One nit: I think || should be on line 1067.

This comment has been minimized.

Copy link
@njsmith

njsmith Apr 20, 2017

Author Contributor

PEP 8 switched to recommending binary operators go on the second line a few months ago (because Knuth says so, among other reasons). Was it decided that PEP 7 should stick with the old way, or...? (I mean I don't care either way, if you want then I'm happy to switch it when I get back to a real keyboard.)

This comment has been minimized.

Copy link
@AraHaan

AraHaan Apr 20, 2017

Contributor

What if the function had some variable it creates before it calls yield from on it?

like for example:

# snip
    def generator1(self):
        something = None
        return (yield from self.generator2())

# snip

This comment has been minimized.

Copy link
@njsmith

njsmith Apr 20, 2017

Author Contributor

@AraHaan: I don't think I understand... Can you elaborate on what you're worried about? The bug we're trying to fix here is that while the yield from is running, KeyboardInterrupt should only be raised inside generator2, not generator1.

I guess there is a small annoyance that if a KeyboardInterrupt arrives at just the wrong moment we could get a spurious "coroutine was not awaited" warning, but even I can't get too worked up about that...

This comment has been minimized.

Copy link
@1st1

1st1 Apr 21, 2017

Member

@njsmith Can you fix the || operator per my last comment? Also, maybe you can expand the "when re-entering a 'yield from'" piece by explaining that generator/coroutine frames are paused at the opcode, and a therefore we might have a chain of frames all waiting on their YIELD_FROM opcode and we don't want to break that chain with an interrupt.

@vstinner
Copy link
Member

left a comment

The change must be documented in Misc/NEWS. IMHO it deserves to be mentionned in Doc/whatsnew/3.7.rst as well.

_testcapi = support.import_module('_testcapi')


class SignalAndYieldFromTest(unittest.TestCase):

This comment has been minimized.

Copy link
@vstinner

vstinner Apr 21, 2017

Member

Please add a reference to the bpo in a comment.

This comment has been minimized.

Copy link
@vstinner

vstinner Apr 21, 2017

Member

Maybe add also a short description of the purpose of this complex test?

if (!PyArg_ParseTuple(args, "O!", &PyGen_Type, &gen))
return NULL;

/* To test what happens if a signal arrives just as we're in the process

This comment has been minimized.

Copy link
@vstinner

vstinner Apr 21, 2017

Member

Please add a reference to the bpo in a comment.

@@ -1064,9 +1064,11 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
Py_MakePendingCalls() above. */

if (_Py_atomic_load_relaxed(&eval_breaker)) {
if (_Py_OPCODE(*next_instr) == SETUP_FINALLY) {
if (_Py_OPCODE(*next_instr) == SETUP_FINALLY

This comment has been minimized.

Copy link
@vstinner

vstinner Apr 21, 2017

Member

Maybe store _Py_OPCODE () result in a local variable to ensure that it's only computed once?

This comment has been minimized.

Copy link
@1st1

1st1 Apr 21, 2017

Member

I think -O2 will do that in this case, no?

This comment has been minimized.

Copy link
@njsmith

njsmith Apr 23, 2017

Author Contributor

Yeah, any compiler will trivially handle this, so I'd rather keep the more straightforward code.

gcc and clang both produce identical code for the local variable and no-local-variable code on -O1 and higher: https://godbolt.org/g/AJY9Zi

bpo-30039: Don't run signal handlers while resuming a yield from stack
If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all.

The included test fails before this patch, but passes afterwards.

@njsmith njsmith force-pushed the njsmith:no-signal-handlers-during-yield-from branch from d9065a6 to 7126cc1 Apr 23, 2017

@njsmith

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2017

I've made all the requested changes and added a NEWS entry. I didn't adds a whats-new entry because I couldn't figure out where to put it :-).

@Mariatta

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

🤔 I think it can go under Other Language Changes.
@1st1 @Haypo Any opinion of where in What's New this should go?

@serhiy-storchaka

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

If this is a bug fix no What's New entry is needed.

@njsmith

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2017

It is a bugfix, and should probably be backported to all supported 3.x branches. My preference would be to put it in and let the whats-new editors figure out how/whether to mention it later.

@1st1

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I'm ok with classifying this as a bugfix. It's totally safe to backport it to 3.6 and 3.5.

@1st1

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I can commit this myself later.

@vstinner

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

If this is a bug fix no What's New entry is needed.

I agree. I will follow @1st1 opinion on how to apply this change and how to backport it or not.

I agree that the change on ceval.c seems safe.

@1st1

1st1 approved these changes May 17, 2017

The PR has been updated with the NEWS entry

@1st1 1st1 merged commit ab4413a into python:master May 17, 2017

3 checks passed

bedevere/issue-number Issue number 30039 found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

1st1 added a commit that referenced this pull request May 17, 2017

[3.6] bpo-30039: Don't run signal handlers while resuming a yield fro…
…m stack (GH-1081)

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all..
(cherry picked from commit ab4413a)

1st1 added a commit that referenced this pull request May 17, 2017

[3.5] bpo-30039: Don't run signal handlers while resuming a yield fro…
…m stack (GH-1081)

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all..
(cherry picked from commit ab4413a)

1st1 added a commit that referenced this pull request Jun 9, 2017

[3.6] bpo-30039: Don't run signal handlers while resuming a yield fro…
…m stack (GH-1081)

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all..
(cherry picked from commit ab4413a)

1st1 added a commit that referenced this pull request Jun 9, 2017

[3.6] bpo-30039: Don't run signal handlers while resuming a yield fro…
…m stack (GH-1081)

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all..
(cherry picked from commit ab4413a)

1st1 added a commit that referenced this pull request Jun 9, 2017

[3.6] bpo-30039: Don't run signal handlers while resuming a yield fro…
…m stack (GH-1081) (#1640)

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all..
(cherry picked from commit ab4413a)

ma8ma added a commit to ma8ma/cpython that referenced this pull request Jun 13, 2017

Merge commit 'master' into pep539-tss-api
Resolve conflcts:
ab4413a bpo-30039: Don't run signal handlers while resuming a yield from stack (python#1081)

mlouielu added a commit to mlouielu/cpython that referenced this pull request Jun 15, 2017

bpo-30039: Don't run signal handlers while resuming a yield from stack (
python#1081)

If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:

- call send() on the outermost generator
- this enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- which calls send() on the next generator
- which enters _PyEval_EvalFrameDefault, which re-executes the
  YIELD_FROM opcode
- ...etc.

However, every time we enter _PyEval_EvalFrameDefault, the first thing
we do is to check for pending signals, and if there are any then we
run the signal handler. And if it raises an exception, then we
immediately propagate that exception *instead* of starting to execute
bytecode. This means that e.g. a SIGINT at the wrong moment can "break
the chain" – it can be raised in the middle of our yield from chain,
with the bottom part of the stack abandoned for the garbage collector.

The fix is pretty simple: there's already a special case in
_PyEval_EvalFrameEx where it skips running signal handlers if the next
opcode is SETUP_FINALLY. (I don't see how this accomplishes anything
useful, but that's another story.) If we extend this check to also
skip running signal handlers when the next opcode is YIELD_FROM, then
that closes the hole – now the exception can only be raised at the
innermost stack frame.

This shouldn't have any performance implications, because the opcode
check happens inside the "slow path" after we've already determined
that there's a pending signal or something similar for us to process;
the vast majority of the time this isn't true and the new check
doesn't run at all.
@isaiah
Copy link
Contributor

left a comment

Not sure how to handle this, should I create a bug on bpo instead?

@@ -9,6 +9,35 @@

from test import support

_testcapi = support.import_module('_testcapi')

This comment has been minimized.

Copy link
@isaiah

isaiah Dec 19, 2017

Contributor

This broke the specs on other implementation, which don't have a _testcapi module.
It should always be guarded by try: except ImportError and skip the spec with the unittest.skipIf decorator.

This comment has been minimized.

Copy link
@vstinner

vstinner Dec 19, 2017

Member

Please open a new bug. It seems like there are other bugs, like test_float.py:

    @support.requires_IEEE_754
    def test_serialized_float_rounding(self):
        from _testcapi import FLT_MAX
        ...

This comment has been minimized.

Copy link
@njsmith

njsmith Dec 20, 2017

Author Contributor

Yeah, a new bug is the way to go. And possibly the developers guide should be updated to say whatever the policy is? (What other implementations are you thinking of? I'm a little wary about just skipping tests – this issue this test is checking for can easily exist on other implementations too...)

This comment has been minimized.

Copy link
@isaiah

isaiah Dec 20, 2017

Contributor

Thanks, I'll create a new bug.

@njsmith I'm working on a java implementation which is based on jython. Your concern is valid, the issue might exist as well in other implementations, that's why I didn't suggest @support.cpython_only, but since the helper method from the _testcapi is also new, it should still be guarded by skipIf, or else the whole test crashes.

This comment has been minimized.

Copy link
@isaiah

isaiah Dec 20, 2017

Contributor

@vstinner just checked the one you posted, it works fine, because it's guarded by @support.requires_IEEE_754, and test.support is implemented in python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.