webbrowser: fix propagation of BaseException #5930
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
53afcbb to
e5d03cb
Compare
amurzeau
left a comment
There was a problem hiding this comment.
I've tested this commit on Debian Stable and tests are passing 👍
| # headless mode gets detected by Twitch, so we have to disable it regardless the user config | ||
| headless=False, | ||
| ) | ||
| except BaseExceptionGroup: |
There was a problem hiding this comment.
On recent python version, if there is a single exception, will this still catch it if the BaseExceptionGroup is unwraped ?
I'm not sure if this can really happen though as tests are passing.
There was a problem hiding this comment.
trio.run() is called with strict_exception_groups=True, so only exception groups can be raised from trio.Nursery contexts. That was the whole reason why trio's 0.25 was a breaking change for us and why we now have to set this argument explicitly on older versions of trio.
There are a couple of cases though where we actually want to catch and log non-group exceptions and then continue running the plugin code without retrieving the client-integrity token, namely resolve_executable(), find_free_port_ipv4(), tempfile.TemporaryDirectory(), etc. Let me fix this real quick...
There was a problem hiding this comment.
Indeed I forgot that strict_exception_groups=True option, thanks for the explanation.
Still good on Debian Stable with 0cc9f86.
- Fix `KeyboardInterrupt`/`SystemExit` exceptions not being propagated in nested `ExceptionGroup`s - Fix compatibility with `exceptiongroup<=1.1.1` by avoiding their `catch()` context manager - Add missing test for (re-)raising different exception types
e5d03cb to
0cc9f86
Compare
KeyboardInterrupt/SystemExitexceptions not being propagated in nestedExceptionGroupsexceptiongroup<=1.1.1by avoiding theircatch()context managerFixes #5929
exceptiongroup>=1.1.2fixed their incorrect behavior of raising anExceptionGroupwhen raising non-group-exceptions in the callback function passed to theircatch()context manager:https://github.com/agronholm/exceptiongroup/blob/1.1.2/CHANGES.rst
Due to this change, we can't use the
catch()context manager anymore and we need to catch allBaseExceptionGroups instead and get a sub-group of the exceptions we're looking for.