Fixes #3275; handle reraise of KI or SystemExit without losing err code #3280

CheViana · 2025-06-06T02:19:22Z

Fixes #3275

Handle reraise of KeyboardInterrupt and SystemExit more gracefully when reraising last exception in exception group.

…sing err msg and exit code

for more information, see https://pre-commit.ci

A5rocks

Thanks for doing this!

src/trio/_tests/test_util.py

A5rocks · 2025-06-06T02:22:59Z

src/trio/_tests/test_util.py

-    # and same for SystemExit
+    # and same for SystemExit but verify code too
+    sysexit_exc = SystemExit("preserve error code")
+    sysexit_exc.code = 1  # can't set it in constructor


Use a code that isn't 1 because that's what it defaults to iirc

(Also I think code is the 1st arg of SystemExit?)

Now that I played a bit more with SystemExit, I agree with what this is testing, I think the comment should be changed though.

EDIT: ouch, I saw you just updated things based on my incorrect understanding of things. Sorry!

src/trio/_tests/test_util.py

A5rocks · 2025-06-06T02:25:05Z

src/trio/_util.py

-            raise type(e) from eg
+            new_exc = type(e)(e.args)
+            if isinstance(e, SystemExit):
+                new_exc.code = e.code  # code can't be set in SystemExit constructor


I don't think this is right:

The constructor accepts the same optional argument passed to sys.exit(). If the value is an integer, it specifies the system exit status (passed to C’s exit() function); if it is None, the exit status is zero; if it has another type (such as a string), the object’s value is printed and the exit status is one.

Actually Python's behavior here is interesting. The comment still seems incorrect, but I agree with this extra work:

>>> se = SystemExit("test") >>> se.code 'test' >>> se.code = 1 >>> se SystemExit('test') >>> se.args ('test',) >>> se.code 1

(you probably knew this which is why you added this, and just wrote the comment wrong or something.)

ok I figured why it was not working for me and fixed
systemexit is not supposed to get actual string error msg, it's just SystemExit(4)

Well it supports it so Trio should probably do the right thing! (I do think it's a very silly use case though, and if it were more than a 5 line change then maybe we shouldn't support it)

…trio into fix/issue-3275-sysexit

codecov · 2025-06-06T02:34:35Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00000%. Comparing base (efd785a) to head (bc3a2c5).
⚠️ Report is 55 commits behind head on main.

Additional details and impacted files

@@               Coverage Diff               @@
##                 main        #3280   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             127          127           
  Lines           19257        19258    +1     
  Branches         1301         1301           
===============================================
+ Hits            19257        19258    +1

Files with missing lines	Coverage Δ
src/trio/_tests/test_util.py	`100.00000% <100.00000%> (ø)`
src/trio/_util.py	`100.00000% <100.00000%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

for more information, see https://pre-commit.ci

CheViana · 2025-06-06T02:53:27Z

Thanks for doing this!

you're welcome

please re-review @A5rocks , fixed requested things

failed tests on py3.13 seem unrelated to my changes, some typing mismatch in some other files

A5rocks · 2025-06-06T02:59:29Z

failed tests on py3.13 seem unrelated to my changes, some typing mismatch in some other files

It may have to do with the pytest version bump. I can't find anything named RaisesContext now so... maybe they removed it? (EDIT: to fix it, replace RaisesContext with pytest.RaisesExc)

I still think we can avoid having to increase the minimum required pytest version (though if you want, you can bump in test-requirements.txt) and I actually feel somewhat conflicted about whether we should preserve any SystemExit#code mutations (which don't get reflected in e.args) now that I learned that exists.

I don't really know what review status to mark a review comment as though, so this is just a regular comment.

for more information, see https://pre-commit.ci

A5rocks

This looks good to me! As stated before I'm conflicted about whether to handle mutation of SystemExit's code attribute, but I'm fine if we don't. Users probably don't do that?

CheViana · 2025-06-06T03:21:18Z

failed tests on py3.13 seem unrelated to my changes, some typing mismatch in some other files

It may have to do with the pytest version bump. I can't find anything named RaisesContext now so... maybe they removed it? (EDIT: to fix it, replace RaisesContext with pytest.RaisesExc)

I still think we can avoid having to increase the minimum required pytest version (though if you want, you can bump in test-requirements.txt) and I actually feel somewhat conflicted about whether we should preserve any SystemExit#code mutations (which don't get reflected in e.args) now that I learned that exists.

I don't really know what review status to mark a review comment as though, so this is just a regular comment.

ok fixed it to work without pytest upgrade, as you suggested, thanks for comments!
also updated error code handling, it's now just type(e)(*e.args) and seems to work ok

CheViana · 2025-06-06T03:33:18Z

This looks good to me! As stated before I'm conflicted about whether to handle mutation of SystemExit's code attribute, but I'm fine if we don't. Users probably don't do that?

I don't think one's supposed to instantiate SystemExit class directly at all, or to mutate their fields, one is supposed to use sys.exit. However for the purposes of, pardon, a bit weird things happening in raise_single_exception_from_group (presumably for backward compat 3.9?) , that's probably a change for better to reraise not empty exception of same type, but exception with same code/message as original

I suppose this PR resolves reporter's issue of missing error code so that's a small change for good

don't think I can merge it though so maybe somebody else can merge?

A5rocks · 2025-06-06T03:41:42Z

don't think I can merge it though so maybe somebody else can merge?

Yeah let's make sure others agree (and they can merge it).

A5rocks · 2025-06-06T03:42:16Z

@seowalex I assume this works for you?

A5rocks · 2025-06-09T00:43:47Z

Well if there's any issues, we can make a follow up PR.

trio-bot · 2025-06-09T00:44:04Z

Hey @CheViana, it looks like that was the first time we merged one of your PRs! Thanks so much! 🎉 🎂

If you want to keep contributing, we'd love to have you. So, I just sent you an invitation to join the python-trio organization on Github! If you accept, then here's what will happen:

Github will automatically subscribe you to notifications on all our repositories. (But you can unsubscribe again if you don't want the spam.)
You'll be able to help us manage issues (add labels, close them, etc.)
You'll be able to review and merge other people's pull requests
You'll get a [member] badge next to your name when participating in the Trio repos, and you'll have the option of adding your name to our member's page and putting our icon on your Github profile (details)

If you want to read more, here's the relevant section in our contributing guide.

Alternatively, you're free to decline or ignore the invitation. You'll still be able to contribute as much or as little as you like, and I won't hassle you about joining again. But if you ever change your mind, just let us know and we'll send another invitation. We'd love to have you, but more importantly we want you to do whatever's best for you.

If you have any questions, well... I am just a humble Python script, so I probably can't help. But please do post a comment here, or in our chat, or on our forum, whatever's easiest, and someone will help you out!

Jane R and others added 3 commits June 5, 2025 22:16

Fixes python-trio#3275; handle reraise of KI or SystemExit without lo…

cdbbda9

…sing err msg and exit code

[pre-commit.ci] auto fixes from pre-commit.com hooks

bdda87d

for more information, see https://pre-commit.ci

remove slow decorator and build test reqs for py3.9

ef88df0

A5rocks requested changes Jun 6, 2025

View reviewed changes

Merge branch 'fix/issue-3275-sysexit' of https://github.com/CheViana/…

669e30e

…trio into fix/issue-3275-sysexit

Jane R and others added 2 commits June 5, 2025 22:46

fix args expansion for exceptions

3e53818

[pre-commit.ci] auto fixes from pre-commit.com hooks

a23bde1

for more information, see https://pre-commit.ci

Jane R and others added 3 commits June 5, 2025 23:15

undo pytest 8.4 upgrade, fixed test to use excinfo

540a192

merge precommit stuff

458a9c0

[pre-commit.ci] auto fixes from pre-commit.com hooks

bc3a2c5

for more information, see https://pre-commit.ci

A5rocks approved these changes Jun 6, 2025

View reviewed changes

A5rocks merged commit be7faad into python-trio:main Jun 9, 2025
43 checks passed

Uh oh!

Fixes #3275; handle reraise of KI or SystemExit without losing err code #3280

Fixes #3275; handle reraise of KI or SystemExit without losing err code #3280

Conversation

CheViana commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

A5rocks Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

A5rocks Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

A5rocks Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

A5rocks Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

CheViana Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

A5rocks Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

CheViana commented Jun 6, 2025

Uh oh!

A5rocks commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

A5rocks left a comment

Choose a reason for hiding this comment

Uh oh!

CheViana commented Jun 6, 2025

Uh oh!

CheViana commented Jun 6, 2025

Uh oh!

A5rocks commented Jun 6, 2025

Uh oh!

A5rocks commented Jun 6, 2025

Uh oh!

A5rocks commented Jun 9, 2025

Uh oh!

Uh oh!

trio-bot bot commented Jun 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CheViana commented Jun 6, 2025 •

edited

Loading

A5rocks Jun 6, 2025 •

edited

Loading

codecov bot commented Jun 6, 2025 •

edited

Loading

A5rocks commented Jun 6, 2025 •

edited

Loading