Skip to content

gh-131878: Handle top level exceptions in new pyrepl and prevent of closing it #131910

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

Conversation

sergey-miryanov
Copy link
Contributor

Working on gh-131878 I saw a code paths that swallows exceptions. I propose not to swallow them and bubble it up to main input loop and handle there.

@picnixz
Copy link
Member

picnixz commented Mar 30, 2025

AFAICT, the swallowed exceptions are already suppressed when executing the statements. When would this except be hit? it's only if we're raising in one of the following:

            ps1 = getattr(sys, "ps1", ">>> ")
            ps2 = getattr(sys, "ps2", "... ")
            try:
                statement = multiline_input(more_lines, ps1, ps2)
            except EOFError:
                break

            if maybe_run_command(statement):
                continue

            input_name = f"<python-input-{input_n}>"
            more = console.push(_strip_final_indent(statement), filename=input_name, _symbol="single")  # type: ignore[call-arg]
            assert not more
            input_n += 1

And in this case, I think it's preferrable to make the REPL exit as it's probably in an unrecoverable state at this point.

@sergey-miryanov
Copy link
Contributor Author

I came from this https://github.com/python/cpython/blob/main/Lib/_pyrepl/base_eventqueue.py#L108-L114.

We swallow UnicodeError exception. And this is a reason why gh-131878 asserted in another point. And if we caught UnicodeError here it give us much more information. But here we don't have any "tools" to properly say about exception. And going up to stack I "found" an appropriate place in the simple_interact loop.

OTOH I may be wrong, and we should not do this.

@picnixz
Copy link
Member

picnixz commented Mar 30, 2025

I think the issue is that we did not properly handled the inputs there. Or that the assertion where it crashes is incorrect. I didn't look in detail but I don't know if #131901 is the correct approach (ISTM that we cannot send long unicode strings but we can do it as bytes? and that we can do it for unicode strings with a single unicode code point, whether it's strings or bytes?)

Nevertheless, when there is a crash like that, I think it's better than we use a debugger instead. At this point of the REPL execution, its state may or may not necessarily be recoverable (I'm actually surprised that we're not aborting because we're out of memory but I guess it's to allow debuggers?)

@sergey-miryanov
Copy link
Contributor Author

ISTM that we cannot send long unicode strings but we can do it as bytes? and that we can do it for unicode strings with a single unicode code point, whether it's strings or bytes?

As long it affects only Windows so I changed only windows code path. And for both cases (if input has one or more unicode code path) input "char" encoded to bytes and sent to eventqueue. For one-byte unicode string it is a small pessimization.

For original issue - we have input == "ñ". If we send it as-is - it sends as str and ord("ñ") == 241. And we just append it to buf. And, if we append "ñ" to buf and try to decode it - it will fail, because it is incomplete unicode char.

But if we encode it as bytes then we get two bytes - b'\xc3\xb1', also append them (via extend) to buf and decode will be happy.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@ambv ambv added the needs backport to 3.13 bugs and security fixes label May 5, 2025
@ambv ambv merged commit 99a0d7e into python:main May 5, 2025
50 checks passed
@miss-islington-app
Copy link

Thanks @sergey-miryanov for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @sergey-miryanov and @ambv, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 99a0d7e5b3c6e80f5b9b0ec88ae439d6d620253c 3.13

ambv added a commit to ambv/cpython that referenced this pull request May 5, 2025
… prevent of closing it (pythonGH-131910)

(cherry picked from commit 99a0d7e)

Co-authored-by: Sergey Miryanov <[email protected]>
Co-authored-by: Łukasz Langa <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented May 5, 2025

GH-133445 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 May 5, 2025
ambv added a commit that referenced this pull request May 5, 2025
@sergey-miryanov
Copy link
Contributor Author

@ambv Thanks!

@sergey-miryanov sergey-miryanov deleted the gh-131878-handle-top-level-exceptions-in-the-new-repl branch May 5, 2025 15:55
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.

3 participants