◐ Shell
reader mode source ↗
Skip to content

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

Merged
1st1 merged 4 commits into
python:masterfrom
njsmith:no-signal-handlers-during-yield-from
May 17, 2017
Merged

bpo-30039: Don't run signal handlers while resuming a yield from stack#1081
1st1 merged 4 commits into
python:masterfrom
njsmith:no-signal-handlers-during-yield-from

Conversation

@njsmith

@njsmith njsmith commented Apr 11, 2017

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

@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

njsmith commented Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

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

1st1 commented Apr 20, 2017

Copy link
Copy Markdown
Member

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 April 20, 2017 19:59

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

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 no-signal-handlers-during-yield-from branch from d9065a6 to 7126cc1 Compare April 23, 2017 01:12
@njsmith

njsmith commented Apr 23, 2017

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

@serhiy-storchaka

Copy link
Copy Markdown
Member

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

@njsmith

njsmith commented Apr 24, 2017

Copy link
Copy Markdown
Contributor Author

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

1st1 commented Apr 24, 2017

Copy link
Copy Markdown
Member

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

@1st1

1st1 commented Apr 24, 2017

Copy link
Copy Markdown
Member

I can commit this myself later.

@vstinner

Copy link
Copy Markdown
Member

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 dismissed vstinner’s stale review May 17, 2017 18:14

The PR has been updated with the NEWS entry

@1st1 1st1 merged commit ab4413a into python:master May 17, 2017
1st1 pushed a commit that referenced this pull request May 17, 2017
…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 pushed a commit that referenced this pull request Jun 9, 2017
…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 pushed a commit that referenced this pull request Jun 9, 2017
…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
Resolve conflcts:
ab4413a bpo-30039: Don't run signal handlers while resuming a yield from stack (python#1081)

@isaiah isaiah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants