Trying to connect creates file descriptors and doesn't free them

[Gregwar]

I have a use case where I want to try to connect to some WebSocket that is not always available. I am continuously wrapping my code in a loop that creates the socket and uses it.

However, if the connection keeps on failing with asyncio.TimeoutError, it looks like the number of file descriptios is blowing up, as you can see with below example:

import websockets.client
import asyncio
import os

async def run():
    while True:
        try:
            async with websockets.client.connect('ws://192.168.0.123', ping_interval=1, ping_timeout=.5, open_timeout=1, close_timeout=1) as self.websocket:
                pass
        except asyncio.TimeoutError:
            f = list(os.scandir('/proc/%d/fd' % os.getpid()))
            print('timeout, fd size=%d' % len(f))

asyncio.run(run())

Outputs:

timeout, fd size=8
timeout, fd size=9
timeout, fd size=10
timeout, fd size=11
...

Until it finally reaches "Too many open files".

Is it a bad pattern fo this use case, or is this a bug ?

[Gregwar]

Looks like replacing Exception with BaseException here:
https://github.com/aaugustin/websockets/blob/541f95cdab5d4dd953fc9428c47421b7168ae0b1/src/websockets/legacy/client.py#L666

Solves the problem, because asyncio.CancelledError is a BaseException and not an Exception

[aaugustin]

I'd rather catch asyncio.CancelledError explicitly if needed than BaseException because there's a bunch of other exceptions included in BaseExceptions. I need to review other places where websockets handles asyncio.Cancelled error to understand if this is the proper solution, consistent with websockets' handling of cancellation in general.

[Gregwar]

Ok, thanks
Keep me on touch if you publish a fixed version, I think I'd rather close my PR and keep this issue open and let you handle this properly

[aaugustin]

Let's do this!

[m-novikov]

Full script to reproduce:

import asyncio
import time
import websockets.client
import os
import threading


async def _run_server():
    server = await asyncio.start_server(
            lambda r, w: None, '127.0.0.1', 8888)

    addrs = ', '.join(str(sock.getsockname()) for sock in server.sockets)
    print(f'Serving on {addrs}')

    async with server:
        await server.serve_forever()


def run_server():
    asyncio.run(_run_server())


async def run():
    while True:
        try:
            async with websockets.client.connect('ws://127.0.0.1:8888', ping_interval=1, ping_timeout=.5, open_timeout=1, close_timeout=1) as self.websocket:
                pass
        except asyncio.TimeoutError:
            f = list(os.scandir('/proc/%d/fd' % os.getpid()))
            print('timeout, fd size=%d' % len(f))


if __name__ == "__main__":
    srv_thread = threading.Thread(target=run_server)
    srv_thread.start()
    time.sleep(1)
    asyncio.run(run())

I need to review other places where websockets handles asyncio.Cancelled error to understand if this is the proper solution, consistent with websockets' handling of cancellation in general.

Is common pattern to wrap some functions in asyncio.wait_for which could result in asyncio.CancelledError to propagate to websockets library and result in open sockets.

[aaugustin]

I agree that catching asyncio.CancelledError in addition to Exception is the way to go here.

I wrote a test that is closer in style to other tests (regardless of the fact that this test suite is a tire fire).