Vaskivskyi / asusrouter

API wrapper for communication with ASUSWRT-powered routers using HTTP protocol
Apache License 2.0
63 stars 8 forks source link

aiohttp.ClientSession is unclosed after disconnect calls #483

Closed ethnhll closed 1 month ago

ethnhll commented 5 months ago

Hello!

I noticed when running examples from the documentation, that after calling router.async_disconnect(), there is a message printed to stdout:


async def gather_data(host, username, password, use_ssl):
    """Method to gether the known data"""
    # async with aiohttp.ClientSession() as session:
        # Initialize
    router = AsusRouter(
        hostname=host, username=username, password=password,
        use_ssl=use_ssl,
    )

    # Connect
    await router.async_connect()

    # Get the client list
    clients = await router.async_get_data(AsusData.CLIENTS)
    for client in clients.items():
        print(client)

    # Disconnect
    await router.async_disconnect()

if __name__ == '__main__':
    _host = "asusrouter.com"
    _username = "USERNAME"
    _passwsord = "PASSWORD"
    _use_ssl = False
    loop = asyncio.new_event_loop()
    loop.run_until_complete(gather_data(_host, _username, _password, _use_ssl))

>> Unclosed client session
>> client_session: <aiohttp.client.ClientSession object at 0x10ae37990>

Digging into it, it does not look like aiohttp.client.ClientSessions created by asusrouter.connection.Connectionfunctions are ever expressly closed.

This could be fine if the API required the user to explicitly create a new Session themselves in a context manager and pass that reference to the AsusRouter constructor, but the signature for AsusRouter shows session as being Optional with a default of None.

This also crops up if a user defined ClientSession is passed and then closed before another API call is made. For example, in Connection._make_post_request a new ClientSession is created if none exists or is closed,

        # Check if a session is available
        if self._session is None or self._session.closed:
            # If no session is available, we cannot be connected to the device
            _LOGGER.debug("No session available. Creating a new one")
            # We will create a new session and retry the request
            self.reset_connection()
            self._session = self._new_session()
            # Reconnect
            await self.async_connect()
            # Retry the request
            return await self._make_post_request(endpoint, payload, headers)

but this Session is outside of a user's control to close themselves and Connection does not close that session.

I might have missed some function that makes this a non-issue, in which case I would just ask that the documentation reflect this as an example :)

Vaskivskyi commented 5 months ago

Hello. Sorry for the delay. Yes, the session handling definitely needs some improvement. I will try to do this as soon as I get a bit of free time