fix: shutdown should wait for in flight requests#4702
fix: shutdown should wait for in flight requests#4702mkleczek wants to merge 2 commits intoPostgREST:mainfrom
Conversation
mkleczek
force-pushed
the
graceful-shutdowns
branch
from
8cbf39a to
ed86489
Compare
mkleczek
changed the title
mkleczek
force-pushed
the
graceful-shutdowns
branch
4 times, most recently
from
91774ab to
6972cc4
Compare
test/io/test_io.py
Outdated
| @pytest.mark.xfail(reason="Graceful shutdown is currently failing") | ||
| def test_graceful_shutdown_waits_for_in_flight_request(defaultenv): | ||
| "SIGTERM should allow in-flight requests to finish before exiting" | ||
|
|
||
| with run(env=defaultenv, wait_max_seconds=5) as postgrest: | ||
|
|
||
| def sleep(): | ||
| response = postgrest.session.get("/rpc/sleep?seconds=3", timeout=10) | ||
| assert response.text == "" | ||
| assert response.status_code == 204 | ||
|
|
||
| t = Thread(target=sleep) | ||
| t.start() | ||
|
|
||
| # Wait for the request to be in-flight before shutting down. | ||
| time.sleep(1) | ||
|
|
||
| postgrest.process.terminate() | ||
|
|
||
| t.join() |
There was a problem hiding this comment.
Had no idea this happened, it would have been great to see it failing on CI first, then do the fix commit.
There was a problem hiding this comment.
Also, do we know what happens to the pg connection when this happens? Does it keep running or it's also closed immediately?
There was a problem hiding this comment.
Also, do we know what happens to the pg connection when this happens? Does it keep running or it's also closed immediately?
It keeps running but the whole pool is destroyed very soon by bracket in CLI.runAppCommand.
EDIT: today the request handling thread is killed (ie. an asynchronous exception is thrown). Depending on when it happens it might cause tx rollback (if thrown in the middle of tx) or tx commit (if thrown right after tx).
There was a problem hiding this comment.
Had no idea this happened, it would have been great to see it failing on CI first, then do the fix commit.
The test in the first commit is marked as xfail to make sure it is failing. The second commit then provides a fix.
I've opened a draft pr to demonstrate the failure - here is the test result: https://github.com/PostgREST/postgrest/actions/runs/22969464771/job/66681946995?pr=4708
There was a problem hiding this comment.
If raised a PR #4714 with the io test marked with xfail. I think it is a pretty cool idea to have a PR with failing test first and then another one with a fix.
mkleczek
force-pushed
the
graceful-shutdowns
branch
from
6972cc4 to
2fb5020
Compare
|
Marked it as draft because there is a problem caused by yesodweb/wai#853. |
|
Related: yesodweb/wai#1064 |
mkleczek
force-pushed
the
graceful-shutdowns
branch
9 times, most recently
from
498627e to
405075a
Compare
mkleczek
force-pushed
the
graceful-shutdowns
branch
from
405075a to
c8c75e4
Compare
|
@steve-chavez @wolfgangwalther - this is dependent on First commit in this PR is a change to our Haskell overlay pointing to We need to decide what to do with this one. |
Nice, looks like the maintainer is already replying.
Since replies are fast I think we should aim to merge upstream and not fork/vendor for now. |
mkleczek
force-pushed
the
graceful-shutdowns
branch
from
c8c75e4 to
bcf3e19
Compare
mkleczek
force-pushed
the
graceful-shutdowns
branch
from
bcf3e19 to
2ddd662
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
4 participants
DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.
Fixes #4689