Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork577
Implement connection tracking and graceful shutdown (sync server)#1615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Implement connection tracking and graceful shutdown (sync server)#1615
Conversation
* Implement sync server connection tracking.* Add ServerConnection.close() call for exising connections on server shutdown. This is useful for cleanly terminating/restarting the server process.Issuepython-websockets#1488
darkhaniop commentedApr 6, 2025
BTW, here's a script I used to check this behavior: importthreadingimporttimefromwebsockets.sync.serverimportserve,ServerclassSharedRef:server:Serverdefserver_target(shared_ref:SharedRef):defecho(websocket):formessageinwebsocket:websocket.send(message)withserve(echo,"localhost",8765)asserver:shared_ref.server=serverserver.serve_forever()defmain():shared_ref=SharedRef()server_thread=threading.Thread(target=server_target,args=(shared_ref,))server_thread.start()try:whileTrue:time.sleep(1)exceptKeyboardInterrupt:passifshared_ref.serverisnotNone:# 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. |
Running tox revealed that the "Sync connection tracking and gracefulshutdown" patch introduced "misordered import statements" warning andunused assignments (client) in "with connect(...) as client:" in theadded tests. This commit addresses these code quality messages.
aaugustin commentedApr 6, 2025
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 commentedApr 7, 2025
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 Speaking of similarity to asyncio, I see that the close method in |
Also, update docstrings to explain the added `connections` arg in the`sync.Server()` constructor.PRpython-websockets#1615
Uh oh!
There was an error while loading.Please reload this page.
Hi again, I implemented the connection tracking and graceful server shutdown procedure that I explained inissue #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.pyto add logic for better handling graceful shutdowns. I also added optional argumentscodeandreasonthat are passed to the clients withServerConnection.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 ofconn_handler(). So, I kept it unchanged. I did add an optional argument to the constructor ofServer(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 doesnot have"