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

Add option to get all errors from create_connection in an ExceptionGroup #74166

Closed
bitdancer opened this issue Apr 4, 2017 · 11 comments
Closed

Comments

@bitdancer
Copy link
Member

bitdancer commented Apr 4, 2017

BPO 29980
Nosy @gvanrossum, @terryjreedy, @tiran, @bitdancer, @1st1, @nanjekyejoannah, @iritkatriel

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-04-04.17:12:21.325>
labels = ['expert-asyncio']
title = 'OSError: multiple exceptions should preserve the exception type if it is common'
updated_at = <Date 2020-11-25.23:31:25.518>
user = 'https://github.com/bitdancer'

bugs.python.org fields:

activity = <Date 2020-11-25.23:31:25.518>
actor = 'christian.heimes'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2017-04-04.17:12:21.325>
creator = 'r.david.murray'
dependencies = []
files = []
hgrepos = []
issue_num = 29980
keywords = []
message_count = 5.0
messages = ['291126', '291127', '291288', '340147', '381869']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'terry.reedy', 'christian.heimes', 'r.david.murray', 'aymeric.augustin', 'yselivanov', 'nanjekyejoannah', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue29980'
versions = []

@bitdancer
Copy link
Member Author

bitdancer commented Apr 4, 2017

create_connection will try multiple times to connect if there are multiple addresses returned by getaddrinfo. If all connections fail it inspects the exceptions, and raises the first one if they are all equal. But since the addresses are often different (else why would we try multiple times?), the messages will usually be different. When the messages are different, the code raises an OSError with a list of the exceptions so the user can see them all. This, however, loses the information as to *what* kind of exception occurred (ie: ConnectioRefusedError, etc).

I propose that if all of the exceptions raised are of the same subclass, that that subclass be raised with the multi-message list, rather than the base OSError.

@bitdancer
Copy link
Member Author

bitdancer commented Apr 4, 2017

If all connections fail (not file :)

@terryjreedy
Copy link
Member

terryjreedy commented Apr 7, 2017

I agree. And if the sub-exceptions are different, prepend them to the messages in the OSError list.

@aymericaugustin
Copy link
Mannequin

aymericaugustin mannequin commented Apr 13, 2019

A very similar issue came up here: aaugustin/websockets#593

When raising an exception that gathers multiple sub-exceptions, it would be nice to be able to iterate the exception to access the sub-exceptions.

@tiran
Copy link
Member

tiran commented Nov 25, 2020

The feature request reminds me of python-trio/trio#611. Nathaniel and Yury have been discussing the idea of a MultiError handler for a while.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel
Copy link
Member

iritkatriel commented Apr 14, 2022

Do we want to do this with ExceptionGroup for 3.11?

@gvanrossum
Copy link
Member

gvanrossum commented Apr 14, 2022

Perhaps, though we’d have to add a flag to enable that behavior, since raising EG would be a backwards incompatible change.

@iritkatriel iritkatriel changed the title OSError: multiple exceptions should preserve the exception type if it is common Add option to get all errors from create_connection in an ExceptionGroup Apr 15, 2022
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Apr 15, 2022
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Apr 16, 2022
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Apr 18, 2022
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Apr 19, 2022
@vstinner
Copy link
Member

vstinner commented Apr 19, 2022

Would it be possible to mark the new all_errors parameter as a keyword-only parameter?

@iritkatriel iritkatriel reopened this Apr 19, 2022
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Apr 19, 2022
@pamelafox
Copy link
Contributor

pamelafox commented Jun 17, 2022

I ran into this while using asyncio.create_connection, which is also what was being used in the linked issue above in the websockets library. The referenced code commit fixing this issue is for sockets.py, however, not for the asyncio version.

What do you think of adding to asyncio.base_events.py create_connection, using same keyword arg all_errors? It works nicely in my local test.

@@ -980,7 +981,7 @@ async def create_connection(
             local_addr=None, server_hostname=None,
             ssl_handshake_timeout=None,
             ssl_shutdown_timeout=None,
-            happy_eyeballs_delay=None, interleave=None):
+            happy_eyeballs_delay=None, interleave=None, all_errors=False):
         """Connect to a TCP server.
 
         Create a streaming transport connection to a given internet host and
@@ -1072,6 +1073,8 @@ async def create_connection(
                 if len(exceptions) == 1:
                     raise exceptions[0]
                 else:
+                    if all_errors:
+                        raise ExceptionGroup("create_connection failed", exceptions)
                     # If they all have the same str(), raise one.
                     model = str(exceptions[0])
                     if all(str(exc) == model for exc in exceptions):

Here's my code that uses it:

async def check_ports(host: str, start: int, end: int, max=10):
    found = 0
    for port in range(start, end):
        try:
            future = asyncio.open_connection(host=host, port=port, all_errors=True)
            r, w = await asyncio.wait_for(future, timeout=timeout)
            yield port
            found += 1
            w.close()
            if found >= max:
                return
        except* ConnectionRefusedError:
            pass
        except* asyncio.TimeoutError:
            pass # closed

@iritkatriel
Copy link
Member

iritkatriel commented Jun 17, 2022

@pamelafox Go ahead and create a new issue and PR for this. It's too late for 3.11, but it could go into 3.12 if the asyncio maintainers agree.

Though, you should check all_errors before len(exceptions). If all_errors then you should always raise an ExceptionGroup, even if it's only of size 1.

@pamelafox
Copy link
Contributor

pamelafox commented Jun 17, 2022

@iritkatriel Okay, sounds good, I'll work on that. And yeah I realized after posting that diff that it needs to go earlier. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants