bpo-29988: Only check evalbreaker after calls and on backwards egdes. by markshannon · Pull Request #18334 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Makes sure that __exit__ or __aexit__ is called in (async) with statements, by not handling interrupts during set up of the with block.
We want to make sure that interrupts are always handled eventually, and ideally that they are handled promptly.
Checking eval_breaker on backward edges ensures that they are always handled eventually.
Checking after every explicit call ensures that they are handled promptly in most cases.
https://bugs.python.org/issue29988
Basically this moves the check for eval breaker to a select few points instead of being more frequent.
Any idea what negative consequences this could have?
Generally things like "what code pattern now prevents a ^C from raising a KeyboardInterrupt in a timely fashion" are how I anticipate this kind of change to manifest.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I believe this is good, though I would like other reviewer eyeballs on it than just mine.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs test cases to make sure that it's actually solving the problem its aiming to solve.
The draft ones I wrote at the 2017 core dev sprints can be found at https://github.com/ncoghlan/cpython/pull/2/files (writing these test cases was the original reason we added per-opcode tracing support to the eval loop)
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
Like @gpshead, this approach looks promising to me in principle - I like it a lot better than what I tried back in 2017. Removing the DISPATCH/FAST_DISPATCH distinction is also a nice bonus (effectively the opcodes that call the eval breaker are now the only ones not using fast dispatch).
So if we're super lucky, the test cases from my old branch will "just work" with this new code. And if they still don't pass, well, that's important to know as well :)
@ncoghlan
Adding tests only ensures that https://bugs.python.org/issue29988 doesn't manifest itself under some circumstances.
However, we know that this PR fixes the problem, because there can be no interrupt between the call to __enter__ and the exception handler being installed.
Having said that, a regression test is a good idea.
I've just had a look at your test. The one thing that I don't like is that it uses f_trace_opcodes, which exists purely to test for https://bugs.python.org/issue29988.
I'd like to remove f_trace_opcodes as its behaviour is undefined, because the sequence of bytecodes generated for any Python code is not defined.
Any ideas on how to test it without using f_trace_opcodes?
I've not tested your branch as https://github.com/ncoghlan/cpython/pull/2/files has some unrelated changes to ceval.c so won't rebase cleanly onto this branch.
XayOn added a commit to XayOn/pyrcrack that referenced this pull request
With the new airmon-ng and airodump-ng APIs with async context managers, proper properties for results and interfaces and callable objects, the "simplified" interface that exposed the Pyrcrack class is no longer needed.
Usage is more comprehensive now, and cleanup afterwards should work. Altough examples seem to not wait for aexit to cleanup (thus airmon-ng's created interface would prevail) upon KeyboardInterrupt.
Might be caused by https://bugs.python.org/issue29988 wich relates to bpo-29988 ( python/cpython#18334 )
@ncoghlan any thoughts on markshannon's question above?
I added f_trace_opcodes specifically to enable the injection of exceptions after an arbitrary number of opcodes. The intended usage is the way I used it in the bpo-29988 test case: inject an exception after every bytecode offset and ensure they're all handled "as expected".
I didn't want to require a special build for the testing, nor rely on an undocumented feature, so we went ahead and published it, even though we didn't manage to fix the original bug report.
If we knew of another way to test this kind of thing, we wouldn't have needed to add f_trace_opcodes :)
… that exit or aexit is called in with statments in case of interrupt.
I think this has sat around for long enough, time to merge.
XayOn added a commit to XayOn/pyrcrack that referenced this pull request
With the new airmon-ng and airodump-ng APIs with async context managers, proper properties for results and interfaces and callable objects, the "simplified" interface that exposed the Pyrcrack class is no longer needed.
Usage is more comprehensive now, and cleanup afterwards should work. Altough examples seem to not wait for aexit to cleanup (thus airmon-ng's created interface would prevail) upon KeyboardInterrupt.
Might be caused by https://bugs.python.org/issue29988 wich relates to bpo-29988 ( python/cpython#18334 )