bpo-30039: Don't run signal handlers while resuming a yield from stack#1081
Conversation
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
The change must be documented in Misc/NEWS. IMHO it deserves to be mentionned in Doc/whatsnew/3.7.rst as well.
Sorry, something went wrong.
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.
d9065a6 to
7126cc1
Compare
April 23, 2017 01:12
|
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 :-). |
Sorry, something went wrong.
|
If this is a bug fix no What's New entry is needed. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I'm ok with classifying this as a bugfix. It's totally safe to backport it to 3.6 and 3.5. |
Sorry, something went wrong.
|
I can commit this myself later. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
The PR has been updated with the NEWS entry
…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)
…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)
…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)
Resolve conflcts: ab4413a bpo-30039: Don't run signal handlers while resuming a yield from stack (python#1081)
isaiah
left a comment
There was a problem hiding this comment.
Not sure how to handle this, should I create a bug on bpo instead?
Sorry, something went wrong.
If we have a chain of generators/coroutines that are 'yield from'ing
each other, then resuming the stack works like:
YIELD_FROM opcode
YIELD_FROM opcode
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.