Skip to content

GH-103929: handle long input lines with PyOS_InputHook #103931

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-103929: handle long input lines with PyOS_InputHook #103931

wants to merge 1 commit into from
+5 −3

Conversation

tacaswell
Copy link
Contributor

@tacaswell tacaswell commented Apr 27, 2023 •
edited by bedevere-bot
Loading

If:

  • stdin is in line buffer mode
  • the readline module is not loaded
  • not on windows
  • a GUI toolkit has installd PyOS_InputHook

Then, if the user enters lines longer than 98 characters into input:

  • user calls input(...)
  • user types/pastes a line longer than 98 characters + 1 newline
  • PyOS_InputHook returns
  • the first 99 characters are returned
  • the last character is not a new line so we go back to my_fgets
  • the PyOS_InputHook blocks because despite there being value in the buffer, stdin does not flag itself as ready to be read again
  • user hits enter again and to finish reading the input
  • the extra new line comes out the next time the user calls input

This fixes this bug by passing the currently read number of characters to my_fgets to skip the input hook when reading out a long buffer.

closes #103929

Qustions:

  • I do not know how to test this other than manually
  • It seems to work when stdin is unbuffered, but I do not fully understand why
  • Is the edit to the docs of PyOS_InputHook correct?

@bedevere-bot bedevere-bot mentioned this pull request Apr 27, 2023
Open
@tacaswell tacaswell mentioned this pull request Apr 27, 2023
Closed
@tacaswell tacaswell force-pushed the fix/pyos_inputhook_longlines branch from a63c908 to 1e6fc5b Compare April 27, 2023 22:41
@mdboom
Copy link
Contributor

mdboom commented May 2, 2023

  • I do not know how to test this other than manually

The usual way to test a C API call directly is add a method to the testcapi module. It seems like you could test this through a call to PyOS_StdioReadline, but it's not exactly trivial to set up the FILE objects and buffer them with data etc. I don't see any other tests in testcapi doing anything like that.

@mdboom mdboom requested a review from brandtbucher May 19, 2023 17:10
@mdboom
Copy link
Contributor

mdboom commented May 19, 2023

@brandtbucher: Maybe you have enough context to review this since you've been in readline-adjacent things lately?

@brandtbucher brandtbucher self-assigned this May 19, 2023
@brandtbucher
Copy link
Member

brandtbucher commented May 19, 2023 •
edited
Loading

I'll take a look today next week. Thanks for the PR!

@tacaswell tacaswell force-pushed the fix/pyos_inputhook_longlines branch from 1e6fc5b to e3759ab Compare June 30, 2023 22:51
@tacaswell
Copy link
Contributor Author

rebased to handle conflicts with the sub-processor work.

@tacaswell tacaswell force-pushed the fix/pyos_inputhook_longlines branch from e3759ab to e69cd68 Compare October 17, 2023 18:48
@tacaswell
Copy link
Contributor Author

Rebased again.

@tacaswell tacaswell force-pushed the fix/pyos_inputhook_longlines branch from e69cd68 to 281588a Compare October 17, 2023 18:49
@tacaswell tacaswell force-pushed the fix/pyos_inputhook_longlines branch from 281588a to 5f76a95 Compare December 4, 2023 20:04
@jdtsmith
Copy link

@brandtbucher did you ever get a chance to review this one?

@tacaswell
pythonGH-103929: handle long input lines with PyOS_InputHook
c433296 
If:
 - stdin is in line buffer mode
 - the readline module is not loaded
 - not on windows
 - a GUI toolkit has installd PyOS_InputHook

Then, if the user enters lines longer than 98 characters into `input`:

 - user calls `input(...)`
 - user types/pastes a line longer than 98 characters + 1 newline
 - PyOS_InputHook returns
 - the first 99 characters are returned
 - the last character is not a new line so we go back to my_fgets
 - the PyOS_InputHook blocks because despite there being value in the buffer,
   stdin does not flag itself as ready to be read again
 - user hits enter again and to finish reading the input
 - the extra new line comes out the next time the user calls input

This fixes this bug by passing the currently read number of characters to
`my_fgets` to skip the input hook when reading out a long buffer.

closes python#103929
@tacaswell tacaswell force-pushed the fix/pyos_inputhook_longlines branch from 5f76a95 to c433296 Compare June 10, 2024 21:04
@mdboom
Copy link
Contributor

mdboom commented Jun 11, 2024

@pablogsal, @ambv: If I understand correctly, users are more likely to hit this bug in 3.14 since the new REPL doesn't use readline. Should we try to get this into the next 3.14 beta?

@pablogsal
Copy link
Member

@pablogsal, @ambv: If I understand correctly, users are more likely to hit this bug in 3.14 since the new REPL doesn't use readline. Should we try to get this into the next 3.14 beta?

This fix doesn't apply to the new REPL, so I don't think how this PR could help here

@mdboom
Copy link
Contributor

mdboom commented Jun 11, 2024

@pablogsal, @ambv: If I understand correctly, users are more likely to hit this bug in 3.14 since the new REPL doesn't use readline. Should we try to get this into the next 3.14 beta?

This fix doesn't apply to the new REPL, so I don't think how this PR could help here

Got it. My bad.

@jdtsmith
Copy link

@pablogsal Does that mean the new REPL should/will not have this 100 char limit while PyOS_InputHook is installed?

@tacaswell
Copy link
Contributor Author

The bug that is fixed is in the readline-replacement implementation that Python carries, not in the input hooks, so if there are bugs in the new repl with long lines, they will be different bugs ;)

@jdtsmith
Copy link

jdtsmith commented Aug 14, 2024 •
edited
Loading

I'll take that as a solid maybe ;). Will report if I find similar long-line bugs with active input hooks. Update: re-reading, it seems you mean the bug remains, but that the new REPL has worked around it in another way. Those of us using input-based REPL's the still need this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

@brandtbucher brandtbucher Awaiting requested review from brandtbucher

Assignees

@brandtbucher brandtbucher

Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

non-readline PyOS_StdioReadline when used with PyOS_InputHook fails is buggy with long input
6 participants
@tacaswell @mdboom @brandtbucher @jdtsmith @pablogsal @bedevere-bot