Skip to content

gh-152907: Restore cooked output flags around the input hook in the new REPL - #153389

Merged
pablogsal merged 5 commits into
python:mainfrom
harjothkhara:gh-152907-pyrepl-inputhook-opost
Sep 24, 2026
Merged

pablogsal merged 5 commits into
python:mainfrom
harjothkhara:gh-152907-pyrepl-inputhook-opost

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jul 9, 2026 •

Copy link
Copy Markdown
Contributor

The problem: While the new REPL waits for input, it puts the terminal in raw mode, which turns off the usual \n → \r\n translation. It also runs input hooks during that wait — these are what GUI toolkits (Tkinter, Qt, …) use to keep their event loops alive at the prompt. So when a hook prints something, each line starts where the previous one ended instead of at the left margin, and the text "staircases" down the screen. The old readline REPL didn't do this, and PYTHON_BASIC_REPL=1 still works fine.

The fix: The REPL needs raw mode for drawing its own output, but the input hook runs at a moment when it isn't drawing. So this turns the newline translation back on just around the hook call, then returns to raw mode. Only the output flags are touched, so keystrokes are still not echoed at the prompt.

Checked with a pty that captures the exact bytes a hook writes:

before:  line1\nline2\nline3\n
after:   line1\r\nline2\r\nline3\r\n

There's a new test that fails without the fix, and ./python -m test test_pyrepl passes.

I used AI assistance for this and have reviewed the change.

Refs #152907

… the new REPL

pyrepl clears OPOST for its own cursor rendering but calls PyOS_InputHook
from inside the raw-mode read loop, so output written by an input hook
(GUI toolkit event loops, and any warning/traceback/print they emit) is
emitted with bare '\n' and no '\r'.  Restore the terminal's saved output
flags around the hook call and re-enter raw mode afterwards; only oflag is
toggled so ECHO/ICANON stay off at the prompt.
The Emscripten buildbot has the pty module but no pty devices, so
pty.openpty() raises OSError("out of pty devices").  Guard the test
class the same way Lib/test/test_pty.py does.
Comment thread Lib/_pyrepl/unix_console.py Outdated
cooked.oflag = self.__svtermstate.oflag
self.__input_fd_set(cooked)
try:
posix._inputhook()

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.

Can we return posix._inputhook() here? This keeps the Unix wrapper consistent with the console API and the Windows path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

self.assertFalse(_termios.tcgetattr(slave_fd)[1] & _termios.OPOST)
finally:
console.restore()
os.close(slave_fd)

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.

Nit, feel free to ignore: relying on a background reader plus time.sleep(0.2) to observe the output tends to be a source of buildbot flakiness. Since the TCSADRAIN in __run_input_hook already guarantees the bytes reach the master before the hook returns, can we drain master_fd directly after hook() instead of sleeping?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tried that first but it hangs on macOS — TCSADRAIN there doesn't return until the master side actually reads the output, so the mode switch blocks forever without a reader. Kept the reader thread but dropped the sleep: once the hook returns the output is already drained, so joining the reader is enough.

@harjothkhara harjothkhara Aug 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed the flaky part.
The hook now drains the PTY master itself, so the test can compare the complete output directly:

def fake_hook():
    observed["oflag"] = _termios.tcgetattr(slave_fd)[1]
    os.write(slave_fd, b"line1\nline2\n")
    observed["output"] = drain()
    return 0

Draining after hook() returns is too late because __run_input_hook switches the terminal mode on the way in and on the way out, so it can block inside the call. The test drains after prepare() and again inside the hook.
The only remaining drainer thread is for teardown, where restore() can block before the caller gets a chance to drain.
On macOS 26.5, tcsetattr(TCSADRAIN) on a PTY slave waits until the master's unread data is drained.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread Lib/_pyrepl/unix_console.py Outdated
return self.__run_input_hook

def __run_input_hook(self):
# Input-hook callbacks (GUI toolkit event loops, and any warning,

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.

This comment is too verbose, pls, simplify it a bit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@unittest.skipIf(is_android or is_apple_mobile or is_wasm32,
"pty is not available on this platform")
class TestUnixConsoleInputHook(TestCase):
# gh-152907: pyrepl runs with OPOST disabled so it can drive the cursor

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.

Thhese AI comments are too verbose pls keep only the relevant parts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, trimmed them down.

harjothkhara and others added 2 commits July 11, 2026 11:22
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The reader thread no longer feeds any assertion: the hook drains the pty
master itself, so the check is an exact comparison. A drainer is still
needed for restore(), which writes before switching modes.
@pablogsal pablogsal added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Sep 24, 2026
@pablogsal
pablogsal merged commit 46ee358 into python:main Sep 24, 2026
65 checks passed
@miss-islington-app

Copy link
Copy Markdown

Thanks @harjothkhara for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖

@miss-islington-app

Copy link
Copy Markdown

Sorry, @harjothkhara and @pablogsal, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 46ee3580c0d1c8b2d527d13b6956881b61b26d34 3.14

@bedevere-app

bedevere-app Bot commented Sep 24, 2026

Copy link
Copy Markdown

GH-158112 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label Sep 24, 2026
@miss-islington-app

Copy link
Copy Markdown

Sorry, @harjothkhara and @pablogsal, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 46ee3580c0d1c8b2d527d13b6956881b61b26d34 3.13

@bedevere-app

bedevere-app Bot commented Sep 24, 2026

Copy link
Copy Markdown

GH-158117 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Sep 24, 2026
@bedevere-app

bedevere-app Bot commented Sep 24, 2026

Copy link
Copy Markdown

GH-158119 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Sep 24, 2026
pablogsal pushed a commit that referenced this pull request Sep 24, 2026
…n the new REPL (GH-153389) (#158112)

gh-152907: Restore cooked output flags around the input hook in the new REPL (GH-153389)

* gh-152907: Restore cooked output flags around the input hook in the new REPL

pyrepl clears OPOST for its own cursor rendering but calls PyOS_InputHook
from inside the raw-mode read loop, so output written by an input hook
(GUI toolkit event loops, and any warning/traceback/print they emit) is
emitted with bare '\n' and no '\r'.  Restore the terminal's saved output
flags around the hook call and re-enter raw mode afterwards; only oflag is
toggled so ECHO/ICANON stay off at the prompt.

* Skip the input-hook test on platforms without pty devices

The Emscripten buildbot has the pty module but no pty devices, so
pty.openpty() raises OSError("out of pty devices").  Guard the test
class the same way Lib/test/test_pty.py does.

* Propagate the input hook's return value and drop the sleep from the test



* Trim comments



* Observe the hook's output synchronously instead of via the reader thread

The reader thread no longer feeds any assertion: the hook drains the pty
master itself, so the check is an exact comparison. A drainer is still
needed for restore(), which writes before switching modes.

---------
(cherry picked from commit 46ee358)

Co-authored-by: Harjoth Khara <harjoth.khara@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
pablogsal added a commit that referenced this pull request Sep 24, 2026
…n the new REPL (GH-153389) (#158119)

(cherry picked from commit 46ee358)

Co-authored-by: Harjoth Khara <harjoth.khara@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@pablogsal

Copy link
Copy Markdown
Member

Great catch on the input hook, @harjothkhara. Thanks for the fix! 🤘

pablogsal added a commit that referenced this pull request Sep 24, 2026
…n the new REPL (GH-153389) (#158117)

(cherry picked from commit 46ee358)

Co-authored-by: Harjoth Khara <harjoth.khara@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

2 participants