Websocket request does not respect the timeout settings

[h3llrais3r]

Describe the bug

Kodi integration in home assistant is using aiohttp for checking the connection to the kodi instances via websockets.
Although a default timeout is specified in the code, it still takes a lot of time until the connection timeouts...
After some debugging, it seems that the client connection for the websockets _ws_connect (https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L690) does not pass down the timeout to the actual request (https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L769-L779)
Because of this, when the url is not available, it will take a lot more time than our specified timeout until the CannotConnectError is thrown.
Can you please pass down the timeout in the request call, so that the configured timeout it taken into account?

To Reproduce

Test code:

import asyncio
import time
from pykodi import CannotConnectError, get_kodi_connection

DEFAULT_PORT = 8080
DEFAULT_SSL = False
DEFAULT_TIMEOUT = 5
DEFAULT_WS_PORT = 9090

CONF_HOST = '192.168.0.110'
CONF_PORT = 8090
CONF_WS_PORT = DEFAULT_WS_PORT
CONF_USERNAME = 'kodi'
CONF_PASSWORD = 'kodi'
CONF_SSL = DEFAULT_SSL

async def ping():
    conn = get_kodi_connection(CONF_HOST,CONF_PORT,CONF_WS_PORT,CONF_USERNAME,CONF_PASSWORD,CONF_SSL,timeout=5)
    try:
        start = time.time()
        await conn.connect()
    except CannotConnectError:
        end = time.time()
        print('timout error after %ss' %(end - start))
        print('error')

asyncio.run(ping())

Expected behavior

Connection error after the specified timeout

Logs/tracebacks

See output of test code

Python Version

$ python --version
Python 3.8.5

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.8.4
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: None
Author-email: None
License: Apache 2
Location: c:\tools\github\kodi-ping\.venv\lib\site-packages
Requires: multidict, aiosignal, attrs, async-timeout, yarl, frozenlist, charset-normalizer
Required-by: pykodi, jsonrpc-websocket, jsonrpc-async

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: c:\tools\github\kodi-ping\.venv\lib\site-packages
Requires:
Required-by: yarl, aiohttp

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.8.2
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: c:\tools\github\kodi-ping\.venv\lib\site-packages
Requires: multidict, idna
Required-by: aiohttp

OS

Windows

Related component

Client

Additional context

home-assistant/core#73097
With current code:

When passing down the timeout in the request, timeout is respected.

Code of Conduct

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

[Dreamsorcerer]

Feel free to make a PR with a test. However, it looks like your suggested change is incorrect, as that change would appear to pass a ClientWSTimeout where a ClientTimeout is expected. It may be necessary to have a second timeout parameter or something to pass to that initial request.

Your example appears to pass an int which is deprecated behaviour. I'm a little surprised if that code is in home-assistant, as I thought they enabled warnings.

[h3llrais3r]

Sorry, the timeout settings seems a bit complex as I never worked before with aiohttp, but as far as I see in the 3.8.4 code, the _ws_connect seems to accept a number/float (default = 10.0) as input:

However, as you are saying, the underlying _request seems to expect a ClientTimeout as input:

So I suppose we should convert the timeout in seconds to a ClientTimeout object?
Actually, the code seems to already convert it if it's not a ClientTimeout?

EDIT: I see the new code seems to have switched to ClientWSTimeout and it indeed warns about deprecation.
So the fix should be adding timeout=ClientWSTimeout(ws_close=timeout) in the self.request()?

Besides the fact that it should be a ClientWSTimeout instead of a int/float, I think the issue is still that the timeout is not passed down into the self.request()

[Dreamsorcerer]

Ah, you're right, it seems to be deprecated in v4, but still expects float in v3. Not sure if that is deliberate or not, will have to check on that.

But, to fix in master, we need it to work with those timeouts, and then backport something reasonable to 3.9.

So the fix should be adding timeout=ClientWSTimeout(ws_close=timeout) in the self.request()?

No, ClientWSTimeout has nothing to do with the request method. As far as I can see ws_receive is the timeout when waiting for a websocket to receive a message and ws_close is the timeout when trying to close a websocket. Neither of these are related to the request method. I think we'd need to accept a ClientTimeout object as a second timeout parameter, or possibly do something like inheriting from ClientTimeout, so all the same timeout parameters are available in ClientWSTimeout. I'm not sure which method is more confusing to the user though (e.g. ClientTimeout.total would only relate to the initial request() method, not to the websocket timeouts, which might be confusing)...

[Dreamsorcerer]

@asvetlov Should #3438 and #3946 have been backported? i.e. Should ClientWSTimeout be available and should float arguments be deprecated for both kinds of timeouts in 3.x?

Also, if we are backporting them, should we then remove support for float from master completely?

[h3llrais3r]

Maybe some side information: the aiohttp is not used directly from home assistant itself, but via the jsonrpc_websocket project, which creates the aiohttp.ClientSession and then passes down all arguments in the ws_connect() call via the **self._connect_kwargs.

Since I saw that the timeout is passed down in that ws_connect() call, I digged a bit more in the aiohttp call and found out that the connection timeout works when that timeout parameter was passed down into the request() call of aiohttp.

Should that still be the way to handle timeouts? Or is there another way? (Like setting the timeout on the ClientSession instead of the ws_connect() call?)

[Dreamsorcerer]

Well, if you set a timeout on the ClientSession, then it'd get picked up in the request() call. So, that should work with the current release (you may still want to send a timeout in the ws_connect() call too, as mentioned before, they relate to different things).

Maybe there should still be a way to pass a timeout through ws_connect() in a future release though, but as mentioned, it would need to be a different timeout to what we are currently receiving, as the timeouts need to handle both the websocket timeouts and the HTTP request timeouts.

[h3llrais3r]

It seems that we cannot really alter the ClientSession as it's including the HA client session in the integration.
So we should be able to pass down a connection timeout in the connect() call.

So if I understood correctly what you were saying, there is a need for an additional 'conn_timeout' for the websockets in _ws_connect which can then be passed down in the self.request(...) as a 'ClientTimeout'? Because it's the timeout passed down into the self.request(...) that really limits the wait time for the connection.

[Dreamsorcerer]

So if I understood correctly what you were saying, there is a need for an additional 'conn_timeout' for the websockets in _ws_connect which can then be passed down in the self.request(...) as a 'ClientTimeout'?

Yes, unless we make ClientWSTimeout subclass ClientTimeout, so it can be reused. As mentioned before, I'm not sure which is less confusing to the user.