Skip to content
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

change strict_exception_groups default to True #2886

Merged
merged 30 commits into from
Jan 22, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Nov 24, 2023

this commit ... is a monster. I could try to split it up some things into separate PRs, but a lot of it is hard to separate.

  • See discussion in Remove MultiError and set strict_exception_groups=True by default #2785, which this fixes most of.
  • I've written an implementation for pytest.raises that handles exceptiongroups. This should probably be added upstream, or in a separate package, or do a way less illegal way of interaction. mypy also goes mad on it, as it should. Don't focus on reviewing _exceptiongroup_util.py for now. See Allow pytest.raises cooperate with ExceptionGroups pytest-dev/pytest#11538
  • A ton of tests had to be changed with the functionality change. I started out by passing strict_exception_groups=False, but decided that embracing the future is much better in the long term and it unveiled a bunch of issues.
  • Most tests are fine, but I haven't looked super closely at all instances where exceptiongroups now get raised, or don't, so the tests diff should be looked over very closely I've marked a couple instances where it seemed clearly suspect that exceptions did or didn't get wrapped - even got an instance of triple-wrapped exceptions that probably should be fixed in a separate PR.
  • _assert_raises has been removed. It seems to be a relic with no reason for existing
  • several tests were likewise ... weirdly written, or simply didn't have the almighty power of ExpectedExceptionGroup, and have been simplified.
  • I left actually removing MultiError and changing nurseries etc to raise ExceptionGroups instead to a separate PR. But changed some tests to check for identity with ExceptionGroup instead of MultiError. (ignore the name of the branch >.>)

This is a pretty big change, and touches on lots and lots of stuff, and should figure out a solution for raises/ExpectedExceptionGroup, so I expect this to stay as a draft for quite a while - maybe with other PRs splitting out from it and implementing parts of it or fixing weird [non-]wrapping.

@jakkdl jakkdl requested a review from Zac-HD November 24, 2023 17:38
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e4c8eb2) 99.64% compared to head (0b45ec9) 99.64%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2886   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files         116      116           
  Lines       17477    17499   +22     
  Branches     3133     3147   +14     
=======================================
+ Hits        17415    17437   +22     
  Misses         43       43           
  Partials       19       19           
Files Coverage Δ
src/trio/_core/_run.py 99.66% <ø> (ø)
src/trio/_core/_tests/test_ki.py 97.83% <100.00%> (+<0.01%) ⬆️
src/trio/_core/_tests/test_run.py 100.00% <100.00%> (ø)
src/trio/_subprocess.py 100.00% <100.00%> (ø)
src/trio/_tests/test_highlevel_open_tcp_stream.py 100.00% <100.00%> (ø)
src/trio/_tests/test_highlevel_serve_listeners.py 100.00% <100.00%> (ø)
src/trio/_tests/test_signals.py 100.00% <100.00%> (ø)
src/trio/_tests/test_socket.py 100.00% <100.00%> (ø)
src/trio/_tests/test_ssl.py 99.85% <100.00%> (ø)
src/trio/_tests/test_subprocess.py 99.23% <100.00%> (+0.04%) ⬆️
... and 4 more

@jakkdl
Copy link
Member Author

jakkdl commented Nov 24, 2023

Other than ExpectedExceptionGroup not being final, all tests pass for me locally. Remains to see if I messed up some mac/windows/pypy-only test

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.

Some initial comments

Comment on lines 806 to 807
of ``TaskGroup`` in asyncio and anyio. This is also to avoid any bugs caused by only
catching one type of exceptions/exceptiongroups.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of ``TaskGroup`` in asyncio and anyio. This is also to avoid any bugs caused by only
catching one type of exceptions/exceptiongroups.
of ``TaskGroup`` in asyncio and anyio. This is also to avoid any bugs caused by only
catching single exceptions in a concurrent scenario.

I'm not thrilled about either wording (proposed or original) though.

Copy link
Member

Choose a reason for hiding this comment

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

I think that we'll eventually want to rewrite this whole section from the new strict-by-default perspective, under the heading "Deprecated: non-strict ExceptionGroups" - to explain that it only exists for backwards-compatibility, will be removed in future, and that we recommend against it for all new code.

I don't want to delay this PR further though, so let's open an issue to revisit the docs on this topic rather than waiting to merge.

src/trio/_core/_tests/test_ki.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_run.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_run.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_run.py Outdated Show resolved Hide resolved
src/trio/testing/_exceptiongroup_util.py Outdated Show resolved Hide resolved
src/trio/testing/_exceptiongroup_util.py Outdated Show resolved Hide resolved
src/trio/testing/_exceptiongroup_util.py Outdated Show resolved Hide resolved
src/trio/testing/_exceptiongroup_util.py Outdated Show resolved Hide resolved
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Nice work!

I've left my thoughts on the raises() context manager over in pytest-dev/pytest#11538 (comment).

If you could pull out the _assert_raises removal and any other test improvements that don't need this to be merged, I'd love to get them in sooner and reduce the size of this diff 🙂

@@ -0,0 +1,2 @@
``strict_exception_groups`` now defaults to True in ``trio.run`` and ``trio.start_guest_run``, as well as ``trio.open_nursery`` as a result of that.
Copy link
Member

Choose a reason for hiding this comment

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

As as result of that what?

I'd also use rst cross-references here.

Copy link
Member Author

Choose a reason for hiding this comment

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

kinda awkward wording, but should be better now

Comment on lines 2210 to 2212
strict_exception_groups (bool): Unless set to false, nurseries will always wrap
even a single raised exception in an exception group. This can be overridden
on the level of individual nurseries.
Copy link
Member

Choose a reason for hiding this comment

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

I prefer this to the phrasing in the open_nursery() docs above - replace that with this? It would also be good to note that strict_exception_groups=False is deprecated and will eventually be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's important to add that "=False is deprecated, and will be removed in a future version of Trio."

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 think we need to add a DeprecationWarning when specifying False to use a wording like that. But if we wanna do that as well in the same release I can get going on it in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

added note about =False being deprecated&removed in the future, and updated open_nursery docstring.

src/trio/_core/_tests/test_run.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_run.py Outdated Show resolved Hide resolved
src/trio/_core/_tests/test_run.py Outdated Show resolved Hide resolved
docs/source/reference-core.rst Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented Dec 11, 2023

Reviewed two cases where I thought ExceptionGroups were missing, and they turned out to be fairly simple cases. Next steps:

@CoolCat467
Copy link
Member

pre-commit.ci autofix

@jakkdl
Copy link
Member Author

jakkdl commented Dec 14, 2023

Ah right, a plethora of PT012. Well, first gotta settle on what message to add to them. PT017 in the helper is pretty amusing.

Comment on lines 946 to 952
assert RaisesGroup(KeyError, ValueError).matches(excinfo.value.__cause__)
# TODO: triple-wrapped exceptions ?!?!
assert RaisesGroup(RaisesGroup(RaisesGroup(KeyError, ValueError))).matches(
excinfo.value.__cause__
)
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the triple-wrapping turns out to be quite straightforward:

trio/src/trio/_core/_run.py

Lines 1903 to 1913 in aadd1ea

async def init(
self,
async_fn: Callable[[Unpack[PosArgT]], Awaitable[object]],
args: tuple[Unpack[PosArgT]],
) -> None:
# run_sync_soon task runs here:
async with open_nursery() as run_sync_soon_nursery:
# All other system tasks run here:
async with open_nursery() as self.system_nursery:
# Only the main task runs here:
async with open_nursery() as main_task_nursery:

doesn't make for the cleanest __cause__s on TrioInternalErrors, but don't think it's a problem. Updating comments.

Copy link
Member Author

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

I've been procrastinating on cleaning up the language in docstrings/documentation. But other than that I think this is the only remaining problem, where run_process sometimes raises an exceptiongroup and sometimes doesn't.
There's maybe better solutions, but I'd just wrap the nursery call in a try/except and raise a new error with the caught exceptiongroup as a cause, as I described in the comment in _subprocess.py.

Comment on lines 763 to 768
# TODO: if this nursery raises an exceptiongroup, manually raise a CalledProcessError,
# or maybe a InternalTrioError, with the exceptiongroup as a cause, so run_process
# never raises an exceptiongroup. (?)
async with trio.open_nursery() as nursery:
# options needs a complex TypedDict. The overload error only occurs on Unix.
proc = await open_process(command, **options) # type: ignore[arg-type, call-overload, unused-ignore]
Copy link
Member Author

Choose a reason for hiding this comment

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

.

Copy link
Member

Choose a reason for hiding this comment

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

(surfacing from an ~hour-long dive into run_process()) I think it's sufficiently unlikely to get an ExceptionGroup here from any reasonable usage, that I'd favor raise InternalTrioError from the_group.

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting multiple exceptions from inside the nursery may be unlikely, but it seems relatively straightforward to pass a bad command that will get an exception from open_process - and we probably don't want to raise InternalTrioError in those cases. Pushing a commit with a suggested fix, but I suspect there might be a reason the open_process call was placed inside the nursery in the first place (?).

Copy link
Member Author

Choose a reason for hiding this comment

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

(the PermissionError in the test is raised inside open_process, and there's validation of inputs that run_process punts to open_process as well)

src/trio/_tests/test_subprocess.py Outdated Show resolved Hide resolved
@jakkdl jakkdl marked this pull request as ready for review January 8, 2024 14:02
Copy link
Member Author

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

Thorough rewrite of docs and an eventual DeprecationWarning for setting strict_exception_groups=False will have to wait for another PR. But other than uncertainty on messing with the nursery in open_process I think everything has been resolved.

src/trio/_subprocess.py Outdated Show resolved Hide resolved
@jakkdl jakkdl requested review from A5rocks and TeamSpen210 January 8, 2024 14:14
@richardsheridan
Copy link
Contributor

richardsheridan commented Jan 11, 2024 via email

@richardsheridan
Copy link
Contributor

richardsheridan commented Jan 11, 2024 via email

@jakkdl
Copy link
Member Author

jakkdl commented Jan 11, 2024

oh and I meant the nursery in _subprocess line ~771

huh, then I'm confused.

@jakkdl
Copy link
Member Author

jakkdl commented Jan 11, 2024

oh and I meant the nursery in _subprocess line ~771

huh, then I'm confused.

I added a test which is doing what you described, afaict, and it's working as expected.

@CoolCat467 CoolCat467 linked an issue Jan 19, 2024 that may be closed by this pull request
@jakkdl
Copy link
Member Author

jakkdl commented Jan 21, 2024

Any remaining semi-blockers have been cleared for this:

So I'll rebump review request from @A5rocks and/or @TeamSpen210. But unless @Zac-HD finds anything else on a re-review I think this can be merged.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks great! Since this PR has been open and at times growing for two months, I'm going to merge, and open an issue for the docs followup mentioned above. Post-merge reviews entirely welcome; I suggest comments here if you want to discuss or just open an issue for some followup action if you spot something we should change further.

Huge thanks to @jakkdl for all your work on this, and to @A5rocks, @CoolCat467, @richardsheridan, and @TeamSpen210 for reviews 🤩 I'm really looking forward to getting this into production at work, and seeing the ecosystem improve handling of concurrent errors!

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.

Exception semantics of run_process with strict_exception_groups
5 participants