-
Notifications
You must be signed in to change notification settings - Fork 178
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to adjust shutdown timeout #56
Conversation
IIUC, you are looking for a way to override the default specified in waitress.task.ThreadedTaskDispatcher.shutdown(). Is that correct? |
Yes, that's correct.
|
Docs changes look good, as does most of the testing. One thing which would be good (but hard) to test is to assert something in 'waitress.test_server' showing that the non-default value was actually used when calling 'shutdown()' on the task dispatcher. |
…ccessfully passed to the task dispatcher shutdown() method.
Agreed. Would the above commit suffice? |
That test looks right to me. |
I'd like to wait a bit on a reply to #48 before applying this, as I think both issues are after the same thing: a way to gracefully shut down the server after finishing pending requests. |
We indeed solve the same problem with different approaches. The timeout ensure that the server will shutdown but you can loose remaining open channels/http requests. The approach #48 is the opposite. I don't see how we could merge both. It's up to you @mcdonc (and other contributors) to choose which one seems the more appropriate :) |
Well, the issue is that the code that this pull request (#56) aims to extend already exists in Waitress and if we just made it possible to adjust the shutdown wait period, I'm not sure #48 would make sense anymore. I don't think anyone actually wants their server to wait forever to shut down, so I suspect some timeout value is necessary, however, for those people who actually do want to wait forever, we could make it so the value "0" for the shutdown wait timeout would mean "wait forever". I'd rather do that than merge #48 at this point unless someone can give me a reason that's a poor idea. |
I would go for your proposition also, seems great with the "timeout = 0 feature". |
After spending some time trying to merge the both PR into clean code I understood that it can't be done (see #48 comments). But I can be wrong. |
Guys, it's 2017... And there is still not configurable graceful shutdown period. This is sad. The change looks almost trivial. |
@tdbear We cannot use this code as the author has not signed the CONTRIBUTORS.txt file. If you'd like to reimplement the feature on the current master you're welcome to and I'm sure it will be reviewed quickly. The project has undergone a change in maintainership since this ticket was open as well as some significant refactorings to support other features and I'm sure an updated PR would be reviewed quickly. As far as it being "sad" - you are welcome to use any WSGI server on the market and we aren't being paid for this work so please keep your contributions focused in the form of technical details / code / docs / pull requests or sponsorship. |
Closing due to original submitter not signing CONTRIBUTORS.txt. |
This pull request adds the ability to specify a shutdown timeout in both the serve() API and the waitress-serve CLI. This defines the maximum amount of time the server will wait for request threads to complete and exit during the shutdown process. The default timeout was and remains 5 seconds.