gh-123856: Fix PyREPL failure when a keyboard interrupt is triggered after using a history search #124396
Conversation
There was a problem hiding this comment.
LGTM.
I confirm that this change fix #123856. The fix works with forward and backward isearch.
Moreover, CTRL+C in an interactive search (CTRL+S or CTRL+R) still resets the prompt to >>> as expected.
Using r.isearch_trans looks safe to me since r is created by _ReadlineWrapper.get_reader() which returns a ReadlineAlikeReader and ReadlineAlikeReader inherits from HistoricalReader.
|
@vstinner thanks for reviewing the draft version of this - I refactored the clean-up process to better mimic what happens when the search is allowed to clean up after itself (see the PR description for a little more info) |
…gered after using a history search
…-up command; add tests
emilyemorehouse
force-pushed
the
bug/gh-123856-pyrepl-search-ctrl-c-fail
branch
from
5bf83d6 to
7389752
Compare
| @@ -1305,3 +1312,7 @@ def test_readline_history_file(self): | |||
| output, exit_code = self.run_repl("exit\n", env=env) | |||
| self.assertEqual(exit_code, 0) | |||
| self.assertNotIn("\\040", pathlib.Path(hfile.name).read_text()) | |||
|
|
|||
| def test_keyboard_interrupt_after_isearch(self): | |||
| output, exit_code = self.run_repl(["\x12", "\x03", "exit"]) | |||
There was a problem hiding this comment.
Would it be possible to add (module-level) constants for "\x12" and "\x03"?
There was a problem hiding this comment.
We'll do that later as part of more tests for control characters.
Lib/test/test_pyrepl/test_pyrepl.py
Outdated
| @@ -1246,6 +1247,12 @@ def _run_repl( | |||
| env["PYTHON_HISTORY"] = os.path.join(cwd, ".regrtest_history") | |||
| if cmdline_args is not None: | |||
| cmd.extend(cmdline_args) | |||
|
|
|||
| term_attr = termios.tcgetattr(slave_fd) | |||
| term_attr[6][termios.VREPRINT] = 0 # pass through CTRL-R | |||
There was a problem hiding this comment.
What is the hardcoded constant 6?
|
Address sanitizer is failing |
…its own class so it still runs
|
After trying to find which TERM's on address sanitizer, we're still seeing which suggests TERM is unset or empty. Let's just add that case to our TestMain ignore. |
| @@ -1305,3 +1312,7 @@ def test_readline_history_file(self): | |||
| output, exit_code = self.run_repl("exit\n", env=env) | |||
| self.assertEqual(exit_code, 0) | |||
| self.assertNotIn("\\040", pathlib.Path(hfile.name).read_text()) | |||
|
|
|||
| def test_keyboard_interrupt_after_isearch(self): | |||
| output, exit_code = self.run_repl(["\x12", "\x03", "exit"]) | |||
There was a problem hiding this comment.
We'll do that later as part of more tests for control characters.
| if r.input_trans is r.isearch_trans: | ||
| r.do_cmd(("isearch-end", [""])) |
|
Thanks @emilyemorehouse for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…gered after using a history search (pythonGH-124396) (cherry picked from commit c1600c7) Co-authored-by: Emily Morehouse <[email protected]> Co-authored-by: Łukasz Langa <[email protected]>
|
GH-124530 is a backport of this pull request to the 3.13 branch. |
…ggered after using a history search (GH-124396) (#124530) gh-123856: Fix PyREPL failure when a keyboard interrupt is triggered after using a history search (GH-124396) (cherry picked from commit c1600c7) Co-authored-by: Emily Morehouse <[email protected]> Co-authored-by: Łukasz Langa <[email protected]>
The original check in
Lib/_pyrepl/simple_interact.pyfor handling the manual clean up when aKeyboardInterruptis received would result in duplicate calls to clean up the search translator in some scenarios. When the search translator is allowed to exit cleanly without aKeyboardInterrupt, it calls its clean-up function (isearch_end) on its own. If aKeyboardInterruptwas triggered immediately after, the most recent command was stillisearch, which would trigger a similar clean-up routine incorrectly. This now ensures that the clean-up is only executed once.I've also added tests – we had to make modifications to the way the full version of the REPL is run – this now allows the necessary control characters to pass through. Additional overrides for other control characters may be required here as the test suite is bolstered.