aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

Wrong constructor called

Open rgeronimi opened this issue 1 year ago • 3 comments

Describe the bug

The ClientConnectorError class has 2 grand-parent classes: ClientConnectionError and OSError, in that order.

However, it seems to call the constructor of the wrong grand-parent: https://github.com/aio-libs/aiohttp/blob/a379e6344432d5c033f78c2733fe69659e3cff50/aiohttp/client_exceptions.py#L136

Following the Python language specification ,a default super() calls the constructor of the first grand parent class, ClientConnectionError , with the wrong arguments (the constructor is supposed to take a message as first argument). The arguments provided, os_error.errno, and , os_error.strerror suggest that it should call instead the constructor of the second grand parent class, OSError .

But maybe my understanding of these classes is wrong.

To Reproduce

Read the code of the client_exceptions.py file: https://github.com/aio-libs/aiohttp/blob/a379e6344432d5c033f78c2733fe69659e3cff50/aiohttp/client_exceptions.py#L136

Expected behavior

If my theory is true, then the constructor of the second grand-parent class shall be called, through: super(OSError, self).__init__(os_error.errno, os_error.strerror)

Logs/tracebacks

Read the code (see above)

Python Version

$ python --version
Python 3.11.6

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.9.1
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache 2
Location: /opt/homebrew/Caskroom/miniforge/base/envs/work/lib/python3.11/site-packages
Requires: aiosignal, attrs, frozenlist, multidict, yarl
Required-by: aiohttp-jinja2, asyncpraw, asyncprawcore, datasets, httpstan, pystan

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.0.4
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /opt/homebrew/Caskroom/miniforge/base/envs/work/lib/python3.11/site-packages
Requires: 
Required-by: aiohttp, Carl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.9.4
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: /opt/homebrew/Caskroom/miniforge/base/envs/work/lib/python3.11/site-packages
Requires: idna, multidict
Required-by: aiohttp, asyncprawcore

OS

MacOS Sonoma 14.2.1 (23C71)

Related component

Client

Additional context

No response

Code of Conduct

  • [X] I agree to follow the aio-libs Code of Conduct

rgeronimi avatar Jan 26 '24 13:01 rgeronimi

Does this result in some wrong behaviour (like the error message being incorrect)? If so, then a simple test would be great.

Dreamsorcerer avatar Jan 26 '24 17:01 Dreamsorcerer

Yes there is one wrong behavior: the OSError object is not properly initialized with the errno information. So any user of the library catching OSErrors will obtain wrongly initialized objects. This is an exotic case although.

I didn't focus on the error message as aiohttp is (like many Python libraries) very heterogeneous in terms of quality of the exception messages, from none (no message at all despite useful information available) to very chatty. So I don't think the error message is the issue there (and if it were it would require a separate issue to resolve it across several exception types).

rgeronimi avatar Jan 27 '24 07:01 rgeronimi

Yes there is one wrong behavior: the OSError object is not properly initialized with the errno information. So any user of the library catching OSErrors will obtain wrongly initialized objects. This is an exotic case although.

That's fine, if you can add a test showing that, then feel free to make a fix.

Dreamsorcerer avatar Jan 27 '24 12:01 Dreamsorcerer

@dmoklaf It looks like it is working as expected to me. You can dump the order in which python runs init methods via this:

from aiohttp.client_exceptions import ClientConnectorError
for x in ClientConnectorError.mro():
    print(x)

which gives:

<class 'aiohttp.client_exceptions.ClientConnectorError'>
<class 'aiohttp.client_exceptions.ClientOSError'>
<class 'aiohttp.client_exceptions.ClientConnectionError'>
<class 'aiohttp.client_exceptions.ClientError'>
<class 'OSError'>
<class 'Exception'>
<class 'BaseException'>
<class 'object'>

Because none of the intermediate Client*Error classes define their own init method the next interesting one to be called is OSError

You can also see it working via a test case like this:

from aiohttp.client_exceptions import ClientConnectorError

try:
    open('non_existent_file.txt', 'r')
except OSError as exc:
    x  = ClientConnectorError('foo', exc)
    print(x.errno)
    print(x.strerror)

which will print out these values (assuming that text file does not exist)

2
No such file or directory

alexmac avatar May 26 '24 20:05 alexmac

Hi @alexmac , I could reproduce successfully your test case (i.e., with the results you did expect). You are correct, this is a subtle case about how constructors arguments are transmitted in cases of multiple inheritance, that I was not aware of.

Sorry for the noise (but I did learn something !).

rgeronimi avatar Jun 24 '24 06:06 rgeronimi