Skip to content

add helper for unwrapping exception groups with single exception #3240

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

Merged
merged 5 commits into from
Apr 9, 2025

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Apr 2, 2025

#3197 has the fairly classic case of "I have an exceptiongroup, but I know it only contains one exception, so I don't want my user to have to use except*". This was also encountered in trio-websocket (python-trio/trio-websocket#188, python-trio/trio-websocket#192), and we have a long section in the docs talking about how messy this is https://trio.readthedocs.io/en/stable/reference-core.html#designing-for-multiple-errors

This PR introduces a helper that can be used in #3197 and trio-websockets, and can be recommended in the docs as well as by flake8-async. There's several pitfalls around unwrapping groups, so users generally shouldn't have to do that by themselves. See HypothesisWorks/hypothesis#4115 and python-trio/trio-websocket#191

I am very open to design discussion on how exactly this helper should behave - it's largely inspired by the implementation I did in trio-websocket. I am also very open to alternative names for the helper, I don't love raise_single_exception_from_group.

TODO:

Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (1bdafca) to head (fab4bbe).
Report is 109 commits behind head on main.

Additional details and impacted files
@@               Coverage Diff               @@
##                 main        #3240   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             124          124           
  Lines           18773        18838   +65     
  Branches         1269         1276    +7     
===============================================
+ Hits            18773        18838   +65     
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.

@@ -28,6 +28,7 @@
MemoryReceiveChannel as MemoryReceiveChannel,
MemorySendChannel as MemorySendChannel,
open_memory_channel as open_memory_channel,
raise_single_exception_from_group as raise_single_exception_from_group,
Copy link
Contributor

@A5rocks A5rocks Apr 2, 2025

Choose a reason for hiding this comment

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

I don't think this should be publicly exported. I think service nurseries take many (if not all) of its use cases and I'd rather encourage their use whenever we get them.

(At least for now. If we don't end up changing anything then at least we have a 1 line + docs change that exposes this)

Copy link
Member Author

Choose a reason for hiding this comment

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

huh, you specifically said on gitter that a helper like this would be "genuinely useful" after @agronholm mentioned it?

https://matrix.to/#/!OqDVTrmPstKzivLwZW:gitter.im/$DaVcJo3WYQqFT_wFz_qbQsRqVJ3e7k_f8WnTl1ArwUU?via=gitter.im&via=matrix.org&via=ancap.tech

but I'm not entirely opposed to making it private for the moment and letting power users play around with it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the main change since then is I hadn't really realized service nurseries/their implications. (I was vaguely aware of what they would do but hadn't put 2 and 2 together).

Really I thought through how this would do compared to service nurseries while typing up #1521 (comment) and I concluded that this primitive is basically only useful for service-nursery shaped tasks (e.g. for #3197 the service nursery would start a service that runs outcome.acapture(partial(anext, aiter)) (or something like that) and then sends through the memory channel which then promptly gets .unwrap()'ed). Also it vaguely feels like capturing intent is important and service nurseries do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like there might still be scenarios where you want this in more complex stuff going on, depending on exactly how the service-nursery implementation ends up - but we can leave it private for now and see how usage pans out.

@jakkdl jakkdl added the skip newsfragment Newsfragment is not required label Apr 7, 2025
@jakkdl jakkdl requested review from A5rocks and agronholm April 7, 2025 11:19
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Only a question but I think this seems fine.

Comment on lines +380 to +381
If a :exc:`KeyboardInterrupt` is encountered, a new KeyboardInterrupt is immediately
raised with the entire group as cause.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you're not giving SystemExit the same treatment?

Copy link
Member Author

@jakkdl jakkdl Apr 8, 2025

Choose a reason for hiding this comment

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

not really, the reason https://trio.readthedocs.io/en/stable/reference-core.html#designing-for-multiple-errors only mentions KeyboardInterrupt is presumably to hammer the point home that the user has no control of when it may happen - but this helper might as well handle them the same way.

@jakkdl jakkdl enabled auto-merge (squash) April 9, 2025 12:34
@jakkdl jakkdl merged commit b137551 into python-trio:main Apr 9, 2025
43 checks passed
@Zac-HD
Copy link
Member

Zac-HD commented Apr 9, 2025

See python/peps#4357 for some notes on traceback preservation when re-raising one leaf from a group!

@jakkdl
Copy link
Member Author

jakkdl commented Apr 17, 2025

See python/peps#4357 for some notes on traceback preservation when re-raising one leaf from a group!

happy to see the PEP!
Yeah this will eat the traceback on the group, but I'm not even sure that's bad in this case - the traceback on the group should only be of being shuffled around in trio internals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip newsfragment Newsfragment is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants