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

[3.6] bpo-30039: Don't run signal handlers while resuming a yield fro… #1640

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

1st1
Copy link
Member

@1st1 1st1 commented 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)

Misc/NEWS Outdated
the middle of resuming a chain of nested 'yield from' or 'await'
calls, it's now correctly delivered to the innermost frame.

bpo-12414: sys.getsizeof() on a code object now returns the sizes
Copy link
Member

Choose a reason for hiding this comment

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

Missing "- " prefix.

@zware
Copy link
Member

zware commented Jun 9, 2017

@1st1 Can this be merged/closed so that we can eliminate the branch?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

As written on the issue, IMHO the backport is small and safe, so LGTM.

Just pleeeease, fix the NEWS file!

@1st1 1st1 force-pushed the backport-ab4413a-3.6 branch from 49d0942 to f80225c Compare June 9, 2017 20:42
@1st1
Copy link
Member Author

1st1 commented Jun 9, 2017

@Haypo @zware working on it!

…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 1st1 force-pushed the backport-ab4413a-3.6 branch from f80225c to fa09db7 Compare June 9, 2017 20:48
@1st1 1st1 merged commit e89f95b into 3.6 Jun 9, 2017
@1st1 1st1 deleted the backport-ab4413a-3.6 branch June 9, 2017 21:06
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.

5 participants