Implement connection tracking and graceful shutdown (sync server)

[darkhaniop]

Hi again, I implemented the connection tracking and graceful server shutdown procedure that I explained in issue #1488.

Server shutdown behavior

In the current version, the sync server does not have a way of tracking active connections. After launching the handler into a separate thread, the Server instance forgets about that connection. When we want to interrupt the server process with an interrupt, this can cause the process to continue running due to remaining active connections.

I updated src/websockets/sync/server.py to add logic for better handling graceful shutdowns. I also added optional arguments code and reason that are passed to the clients with ServerConnection.close(code, reason) which allows additional information to be provided to the clients.

Considerations

One of the things I considered when choosing how to update active connections is the existing API. I am not sure if there are users who instantiate Server() differently in their codebases, as opposed to the recommended server = serve(socket, handle, ...) approach. Therefore, I think there could be complaints about changing the signature of conn_handler(). So, I kept it unchanged. I did add an optional argument to the constructor of Server (as a keyword-only arg, so I think it won't be a problem either).

I added some tests in tests/sync/test_server.py, and tried to match the style of existing tests there.

Please let me know if anyone has suggestions on this.

Update: fixed a typo "sync server does have" -> "sync server does not have"

[darkhaniop]

BTW, here's a script I used to check this behavior:

import threading
import time
from websockets.sync.server import serve, Server

class SharedRef:
    server: Server

def server_target(shared_ref: SharedRef):

    def echo(websocket):
        for message in websocket:
            websocket.send(message)

    with serve(echo, "localhost", 8765) as server:
        shared_ref.server = server
        server.serve_forever()

def main():
    shared_ref = SharedRef()
    server_thread = threading.Thread(target=server_target, args=(shared_ref,))
    server_thread.start()
    try:
        while True:
            time.sleep(1)
    except KeyboardInterrupt:
        pass

    if shared_ref.server is not None:
        # shared_ref.server.shutdown(reason="scheduled_maintenance")
        shared_ref.server.shutdown()

if __name__ == "__main__":
    main()

In my test, when this server has active connections (with the updated library), KeyboardInterrupt terminates the process without delay or exceptions.

[aaugustin]

I also added optional arguments code and reason that are passed to the clients with ServerConnection.close(code, reason) which allows additional information to be provided to the clients.

Is there a real-world use case for this?

For context, in 12 years of developing this library, I have never come across anyone caring about close codes or reasons. Either you have a good reason and I'm interested, or let's keep the API simple and send a 1001 like the asyncio API does.

[darkhaniop]

Is there a real-world use case for this?

Admittedly, I have not used it in my client scripts either (as in, never made my scripts check the closing message). I added these args because the ServerConnection.close() accepts them, but I don't have strong opinions regarding their usefulness. So, I do not see an issue with not having them.

Speaking of similarity to asyncio, I see that the close method in .asyncio.server.Server accepts an optional close_connections: bool = True arg. Would adding the same option to .sync.Server.shutdown() be helpful?