python-websockets / websockets

Library for building WebSocket servers and clients in Python
https://websockets.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
5.16k stars 513 forks source link

Sync client stops process exit when running in a Thread if not explicitly closed #1455

Closed uglybug closed 5 months ago

uglybug commented 6 months ago

Environment

Python: 3.11.4 (though the version doesn't seem to make a difference) Websockets: 12.0 OS: Ubuntu in WSL2 (though this also happens in MacOS on my work environment)

Description

When running a sync websocket inside a Thread that starts itself, the program is blocked from exiting at the normal exit point if the websocket is not explicitly closed. This seems to be due to a race condition on a lock acquisition.

Recreation Code

This simple program will demonstrate the issue:

from threading import Thread
from websockets.sync import client

class ThreadedClient(Thread):
    def __init__(self):
        super().__init__()
        self.daemon = True
        self.ws = client.connect("wss://echo.websocket.org")
        self.start()

    def run(self):
        for msg in self.ws:
            print(msg)

ws_client = ThreadedClient()

print("This will be printed, proving we don't block on Thread creation...")

# We should exit immediately here as we did not call a blocking method on the thread
# In fact, let's call an explicit exit to prove the point:

exit()

Running this, the program never exits. In fact, if we send it a KeyboardInterruption, then you can see that the stack trace seems to be suggesting that the standard thread library is blocked obtaining a thread lock on exit, for some reason:

Traceback (most recent call last):
  File "/home/simon/.pyenv/versions/3.11.4/lib/python3.11/threading.py", line 1583, in _shutdown
    lock.acquire()
KeyboardInterrupt:

More strangely, running the same code, but with an async client inside an asycio event loop, works fine and exits cleanly (which is actually the workaround I have employed for this issue in my own code for now).

If we explicitly close the socket before exit, then the program exits cleanly. However, this is not a great requirement for a pure background thread that only communicates with the caller via callbacks. Having the caller having to hold a reference to the thread and then explicitly call a close method before exiting is not very ergonomic. Therefore, I am hoping that if the websocket self-closed on __del__ then this would solve the problem with blocking the exit.

uglybug commented 6 months ago

Interestingly, rewriting the above code as below does work as expected. So it only seems to be an issue when you run a websocket inside a class that is a subclass of Thread, rather than in a Thread itself, necessarily:

from websockets.sync import client
from threading import Thread

def ws():
    ws = client.connect("wss://echo.websocket.org")

    for msg in ws:
        print(msg)

t = Thread(target=ws)
t.daemon = True
t.start()

print("Thread started. Now trying to exit")

exit()

# This does exit cleanly as we expect.
uglybug commented 6 months ago

Actually, with a few more tests this morning, I narrowed down the issue a bit more. Seems to be if the client is passed into a Thread that runs the read loop, then that prevents process exit, even if the Thread is a daemon. So, taking the code above that exits successfully, if we simply move the instantiation of the client and pass it into the Thread then the process now cannot exit as long as the client connection is open:

from websockets.sync import client
from threading import Thread

def ws(sock):
    for msg in sock:
        print(msg)

c = client.connect("wss://echo.websocket.org")
t = Thread(target=ws, args=(c,))
t.daemon = True
t.start()

print("Thread started. Now trying to exit")

exit()
# Never exits

This is a pain as it means that we cannot have a continuous read in a daemon thread, whilst keeping the reference to the client so we can asyncronously send. Unless, as I have done in my own code to get around this, we run an async client in a daemon thread (which the documentation states is not supported).

mpetazzoni commented 5 months ago

I was looking into this and the reason this is happening is because the client.connect() call constructs the ClientConnection object, which starts the recv_events_thread. It does so without specifying the daemon parameter to the thread creation: https://github.com/python-websockets/websockets/blob/main/src/websockets/sync/connection.py#L86

This means this thread inherits the daemon status from its parent. So if you want the WebSocket connection's "background" thread to truly be a daemon thread, it must be created from within a thread that is itself a daemon – and the main thread of the Python program is not.

This certainly would be helpful to have noted in the docs of the sync client.

aaugustin commented 5 months ago

Perhaps the background thread should be a daemon thread? If the main thread as well as any other thread managed by the user of the library is done, there's no reason to prevent the program from exiting.


we run an async client in a daemon thread (which the documentation states is not supported).

It works. I'm discouraging it because:

mpetazzoni commented 5 months ago

@aaugustin I agree, thanks for making the change! Let us know when you cut a release.

aaugustin commented 1 month ago

@mpetazzoni I just released version 13.0 which includes this change.

(Yes this library has an irregular release schedule. It's a side project.)