Skip to content

Treat KeyboardInterrupt or SystemExit the same on program exit even if they are inside exception groups #130713

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

Open
A5rocks opened this issue Mar 1, 2025 · 11 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@A5rocks
Copy link

A5rocks commented Mar 1, 2025

Feature or enhancement

Proposal:

A KeyboardInterrupt or SystemExit inside a BaseExceptionGroup should be treated like a bare KeyboardInterrupt or SystemExit.

PS C:\...> python x.py
  + Exception Group Traceback (most recent call last):
  |   File "C:\...\x.py", line 1, in <module>
  |     raise BaseExceptionGroup("ki in an exception group", [KeyboardInterrupt()])
  | BaseExceptionGroup: ki in an exception group (1 sub-exception)
  +-+---------------- 1 ----------------
    | KeyboardInterrupt
    +------------------------------------
PS C:\...> $LastExitCode
1


PS C:\...> python x.py
Traceback (most recent call last):
  File "C:\...\x.py", line 1, in <module>
    raise KeyboardInterrupt()
KeyboardInterrupt
PS C:\...> $LastExitCode
-1073741510

Looking at the CPython source code, I believe _Py_HandleSystemExitAndKeyboardInterrupt is the relevant function.


Points for it:

  • less pitfalls for a 3rd party using exception groups
  • see any other failures (as exceptions) upon shutdown (ATM the workaround requires discarding the other exceptions)

Points against:

  • SystemExit in an exception group prints out the stack trace, unlike when it's not in an exception group
    • we could make them consistent (filter out SystemExit and print the exception group then)

I volunteer to implement this if this is fine.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

EDIT: I have now made a post. See https://discuss.python.org/t/keyboardinterrupt-and-systemexit-in-exception-groups-should-be-considered-for-pythons-exit-code/82816

Links to previous discussion of this feature:

I have tried some basic keyword searches as well as some digging in blame, but I can't find any previous discussion.

@A5rocks A5rocks added the type-feature A feature request or enhancement label Mar 1, 2025
@picnixz picnixz added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Mar 1, 2025
@picnixz
Copy link
Member

picnixz commented Mar 1, 2025

cc @iritkatriel as one of exception groups' PEP author

@ZeroIntensity
Copy link
Member

I think we would need a new PEP for this, and I'm not sure it's worth it. Basically any change to a builtin requires a PEP, because other Python implementations are following the standard (which is, in this case, what PEP-654 specifies), not what we arbitrarily think is correct on our end. See #129867 (comment).

@ZeroIntensity ZeroIntensity added the pending The issue will be closed if no feedback is provided label Mar 1, 2025
@A5rocks
Copy link
Author

A5rocks commented Mar 2, 2025

To be clear this is a change in running a script -- no change in ExceptionGroup or BaseExceptionGroup. This behavior was not mentioned in the PEP.

(I think you misunderstood the issue)

@ZeroIntensity
Copy link
Member

Ah, thanks for clarifying--I was interpreting this as changing the way SystemExit and KeyboardInterrupt propagate.

But, I'm still not quite sure if it's right to special-case these for the return codes. This would still create some deviation between CPython and other implementations, right? (If PEP-654 didn't specify any special case for how these should get handled, then one could argue that it was implicitly specified that they should behave the same.)

@A5rocks
Copy link
Author

A5rocks commented Mar 2, 2025

There is already deviation -- on Windows, pypy exits with exit code 2 not STATUS_CONTROL_C_EXIT:

PS C:\...\pypy3.11-v7.3.19-win64> ./python.exe x.py
Traceback (most recent call last):
  File "C:\...\x.py", line 1, in <module>
    raise KeyboardInterrupt()
KeyboardInterrupt
PS C:\...\pypy3.11-v7.3.19-win64> $LastExitCode
2
PS C:\...\pypy3.11-v7.3.19-win64> python x.py
Traceback (most recent call last):
  File "C:\...\pypy3.11-v7.3.19-win64\x.py", line 1, in <module>
    raise KeyboardInterrupt()
KeyboardInterrupt
PS C:\...\pypy3.11-v7.3.19-win64> $LastExitCode
-1073741510

EDIT: previously I accidentally compared pypy's Python 2 with CPython's Python 3.

EDIT 2: after learning that this behavior was added sometime after Python 3, I found that KeyboardInterrupt's handling was added in #41078 in 2019.

@graingert
Copy link
Contributor

I think we would need a new PEP for this, and I'm not sure it's worth it. Basically any change to a builtin requires a PEP, because other Python implementations are following the standard (which is, in this case, what PEP-654 specifies), not what we arbitrarily think is correct on our end. See #129867 (comment).

I don't think this needs a pep, contextlib.supress was changed without one, and the behaviour of setting the exit code was also added without a pep

@picnixz
Copy link
Member

picnixz commented Mar 2, 2025

contextlib.suppress is not a built-in though? by built-in, we really mean built-in in exceptions.c here

@A5rocks
Copy link
Author

A5rocks commented Mar 2, 2025

by built-in, we really mean built-in in exceptions.c here

Just again as a clarification, this would modify _Py_HandleSystemExitAndKeyboardInterrupt in pythonrun.c. I have not actually implemented this yet, but looking at the code and specifics, I think I would need to change what's currently PyErr_ExceptionMatches to also allow relevant exception groups and either:

  • reorder checks
  • remove the if (_PyRuntime.signals.unhandled_keyboard_interrupt) statement in main.c and instead *exitcode_p = exit_sigint(); in _Py_HandleSystemExitAndKeyboardInterrupt

s.t. BaseExceptionGroup("a", [SystemExit(3), KeyboardInterrupt()]) exits with code 3

This is not proposing changing:

  • ExceptionGroup or BaseExceptionGroup's behavior
  • anything in exceptions.c
  • anything accessible at runtime in a Python script (unless that "thing accessible" is the result code of a subprocess run of another Python invocation.)

@picnixz
Copy link
Member

picnixz commented Mar 2, 2025

Just again as a clarification, this would modify _Py_HandleSystemExitAndKeyboardInterrupt in pythonrun.c

Ok, I thought that it would also warrant a change in the how we represent the base exception groups. Anyway built-ins usually refer to primitive types (such as int, str, exceptions, etc) but also other stuff that I am not aware of. OTOH, a PEP is usually needed when the change is either breaking enough and/or is big and changes the language.

To make BaseExceptionGroup("a", [SystemExit(3), KeyboardInterrupt()]) exist with code 3, we would change how the interpreter interpretes this upon exit, namely, which exception should be considered as the "important" one here. So, while it doesn't warrant a PEP at first glance, it could be a big enough change as other implementations of Python (the language) may decide otherwise. In this case, a PEP is needed.

And then, without considering which code is to be affected, the fact that the specs (the PEP) did not explicitly explain how to handle such exceptions upon exit may still warrant a new PEP. Or as @ZeroIntensity said, "then one could argue that it was implicitly specified that they should behave the same", in which case, we should treat this as a bug fix.

But I don't know whether it's better to be implicit or explicit.

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Mar 2, 2025

unless that "thing accessible" is the result code of a subprocess run of another Python invocation

Yeah, that's what I'm concerned about. A return code is technically part of the API--it's used in tests and in CI to check if something failed. This might be niche enough that we don't have to worry about it, but for now, I'm inclined to point this to DPO first.

@A5rocks
Copy link
Author

A5rocks commented Mar 2, 2025

I'll do that and edit a link in -- should the pending tag be removed?

@ZeroIntensity ZeroIntensity removed the pending The issue will be closed if no feedback is provided label Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants