Implemented graceful shutdown for HTTP requests #2717
Conversation
* Listens for SIGINT as the termination signal * Stops accepting new connections once the signal is received * Waits for all connections to be drained, before shutting down * Forcefully kills all pending connections after 30 seconds Currently this does not send a close message to websocket clients, I'd like to submit that change as a separate pull request, but at least this solve my biggest concern which is not getting confirmation for mutations while restarting the server.
|
Deploy preview for hasura-docs ready! Built with commit f32252e |
|
Review app for commit 52d1659 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
Review app for commit 61f612a deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
Thanks for the contribution! The easiest way to run the integration tests is to do You can also run specific tests with The script is very new so do let us know if it works on your machine. |
|
@jberryman It works on my machine, although I consistently get 2 failures in the master branch. The test I would need to write requires that I kill the Hasura server, which is handled in the shell file and not in the python test harness. How do you suggest I implement this? The natural way would be to have the start/stop function in the python code and be able to start or stop the server per test if required. |
|
@lorenzo I appreciate you trying out the script. I think it's not terribly important to have an automated test for this behavior right now, but I'll let @lexi-lambda chime in with her review. The script you've posted is super helpful though thank you! Tested locally and change LGTM, didn't look closely at the failure we're getting on CI ^ |
|
Review app for commit 1c53104 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
Review app for commit 48c6464 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
@lorenzo This looks good to me, but I have one question: given you’re listening on an arbitrary signal, anyway, why not just listen for |
|
@lexi-lambda I’m fine with that too, just did not want to break backwards compatibility, since the only thing that actually stops the docker container, even before my change, is a SiGINT. I’m fine we removing that line in this PR if you think we should go that way. |
Added a helpful log line to the graceful shutdown sequence
|
@lexi-lambda I change the signal to |
|
Review app for commit 8efb539 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
Review app for commit 4427a25 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
lexi-lambda
left a comment
There was a problem hiding this comment.
These changes seem fine to me. I have a slightly more elaborate approach I’ve been playing with on a branch involving converting SIGTERM signals to asynchronous exceptions (in the same way SIGINT signals are already converted by the RTS to UserInterrupt exceptions), so I might open a PR with that at some point, but I don’t know if there’s actually much of an advantage to it at the moment, anyway.
Thank you for your contribution!
|
@lexi-lambda interesting, what would the advantage of your approach be? |
Mostly just consistency with what GHC already does. Originally I was thinking it might make it easier to implement graceful shutdown for things running on several different threads, but that isn’t actually true, since GHC only throws Anyway, it seems like, interestingly enough, these changes make the CLI tests fail! It looks like, for some reason or another, the graceful shutdown on SIGTERM causes the working tree to become dirty when it didn’t before. I am not immediately sure why that is the case. |
|
Review app for commit c23e58e deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
@lexi-lambda I think it is trivial with this code to install the same signal handler for SIGINT as well. Do you want me to add thet |
|
If you would like, but there’s no need to. |
|
Review app for commit 7ddcca1 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
@lexi-lambda I used my very limited knowledge on what I thought could be wrong in the tests, but I truly don't know what it is :( |
|
@lexi-lambda I finally managed to make the test script pass |
lexi-lambda
left a comment
There was a problem hiding this comment.
Ah, it was .tix files, that makes sense. Good catch.
|
Review app for commit e79d3b9 deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
Review app for commit f32252e deployed to Heroku: https://hge-ci-pull-2717.herokuapp.com |
|
Review app https://hge-ci-pull-2717.herokuapp.com is deleted |
…asura#2717) * Listens for SIGTERM as the termination signal * Stops accepting new connections once the signal is received * Waits for all connections to be drained, before shutting down * Forcefully kills all pending connections after 30 seconds Currently this does not send a close message to websocket clients, I'd like to submit that change as a separate pull request, but at least this solve my biggest concern which is not getting confirmation for mutations while restarting the server.
Description
Currently this does not send a close message to websocket clients, I'd
like to submit that change as a separate pull request, but at least this
solve my biggest concern which is not getting confirmation for mutations
while restarting the server.
Affected components
Related Issues
refs #2698
Steps to test and verify
I haven't codified this as a test, although I'm happy to provide it if you guide me in running the test suite and your preferred solution. I did this:
Save this to a
test.shfileThen watched it:
Waited for a few requests to go through then killed the server:
Verified that no broken socket errors occur, only the final
connection refusedLimitations, known bugs & workarounds
This currently does not deal with gracefully shutting down websocket connections