Pylons / waitress

Waitress - A WSGI server for Python 3
https://docs.pylonsproject.org/projects/waitress/en/latest/
Other
1.44k stars 164 forks source link

`waitress.server.TcpWSGIServer` leaves daemon thread behind after `close` is called #402

Closed bwindsor closed 1 year ago

bwindsor commented 1 year ago

Example to reproduce the issue:

from flask import Flask
import threading
import waitress
import time

wsgi_app = Flask("flask_name")

print(threading.active_count())  # prints "1"
server = waitress.create_server(wsgi_app, threads=1)

def stop_waitress():
    time.sleep(2)
    server.close()

stop_thread = threading.Thread(target=stop_waitress)
stop_thread.start()

server.run()

stop_thread.join()
print(threading.active_count())  # prints "2"

I would expect that after calling close on the server, threads would be cleaned up rather than left to hang around as daemons. In MultiSocketServer it looks like this issue is solved: https://github.com/Pylons/waitress/blob/afc7b9de54f21b63bb1e8fd6b12c704876d9868a/src/waitress/server.py#L175

But in BaseWSGIServer it does not attempt to shutdown the task_dispatcher as part of the close method. https://github.com/Pylons/waitress/blob/afc7b9de54f21b63bb1e8fd6b12c704876d9868a/src/waitress/server.py#L357-L359

A simple solution would be to add self.task_dispatcher.shutdown() to the close() method.

mmerickel commented 1 year ago

You might look at the StoppableWSGIServer in webtest which is doing the right thing. I’m not saying it shouldn’t be in waitress but incase you weren’t aware this does work.

https://github.com/Pylons/webtest/blob/main/webtest/http.py

bwindsor commented 1 year ago

Thanks - I was aware that something in webtest exists, but this isn't only for the purposes of unit tests, I also need to be able to shut down my production system gracefully. In the system I'm not running waitress in isolation, it's one of a handful of threads doing different things. I could run it in its own process and then kill the process to ensure it's all cleaned up, but that adds extra complexity.

mmerickel commented 1 year ago

You can see several related tickets already in waitress around getting the shutdown to be properly graceful. Your suggestions won’t get it to that point.

bwindsor commented 1 year ago

In that case, let's close this so that I'm not creating unnecessary noise, and leave it to be included in the bigger changes for fully graceful shutdown. Thanks for the quick replies!

mmerickel commented 1 year ago

Thanks! The graceful shutdown is tricky and neither @bertjwregeer nor I have had time to get into the nitty gritty of it, despite it being important.

It requires configuring waitress to stop listening for new connections but keep asyncore open (but stop listening) to continue processing existing connections to a point before ultimately shutting things down when those active requests are complete.