Skip to content
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 --timeout-graceful-shutdown parameter #1950

Merged
merged 16 commits into from
Apr 26, 2023

Conversation

JonasKs
Copy link
Contributor

@JonasKs JonasKs commented Apr 24, 2023

Summary

This is a continuation of #1824, which intend to close:

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

The running_in_executor-tasks problem:

As of 1b3e737, I've removed cancellation of tasks running in executors such as those spawned through run_in_executor by using os._exit(1).

The implementation worked, but unfortunately, it crashes a lot of tests (fixable), but I am also unable to actually make a test for it, and therefor I'm unsure if it will work on all platforms.

The `os._exit` implementation

Given main.py, and commit 4b703e3:

`main.py`
import time
from fastapi import FastAPI
from starlette.background import BackgroundTasks
import uvicorn

app = FastAPI()

def z():
    print('normal sleep')
    time.sleep(10)
    print('normal awake')

@app.get('/')
async def x(background: BackgroundTasks):
    background.add_task(z)

if __name__ == '__main__':
    uvicorn.run(app, timeout_graceful_shutdown=2)
  1. run the server
  2. do a GET request
  3. do cancel server
  4. see process successfully shut down after 2 seconds instead of 10.

How ever, I'm unable to test it. The test below will finish immediately as expected, but the pytest process will run for 10 seconds.

Test file
async def app_run_in_exec(scope, receive, send):
    def long_func_to_be_cancelled():
        time.sleep(10)

    loop = asyncio.get_event_loop()
    loop.run_in_executor(None, long_func_to_be_cancelled)

@pytest.mark.anyio
async def test_thread_task_background(unused_tcp_port: int):
    """
    Ensure background tasks created with `.run_in_executor` is shut down, and not waited for.
    """
    config = Config(app=app_run_in_exec, reload=False, port=unused_tcp_port, timeout_graceful_shutdown=1)
    server: Server

    async with run_server(config) as server:
        # run a thing in an executor
        server.handle_exit(sig=signal.SIGINT, frame=None)

uvicorn/server.py Outdated Show resolved Hide resolved
uvicorn/server.py Outdated Show resolved Hide resolved
@JonasKs JonasKs force-pushed the feat/timeout-graceful-shutdown-cont branch 4 times, most recently from 4450676 to dbb5d96 Compare April 25, 2023 17:38
@JonasKs JonasKs force-pushed the feat/timeout-graceful-shutdown-cont branch from dbb5d96 to 36ef166 Compare April 25, 2023 17:45
@JonasKs JonasKs force-pushed the feat/timeout-graceful-shutdown-cont branch from 293e891 to 47c9357 Compare April 25, 2023 18:17
@JonasKs JonasKs requested a review from Kludex April 25, 2023 18:21
@JonasKs

This comment was marked as outdated.

@Kludex Kludex changed the title Feat/timeout graceful shutdown Add --timeout-graceful-shutdown parameter Apr 25, 2023
docs/settings.md Outdated Show resolved Hide resolved
@JonasKs JonasKs force-pushed the feat/timeout-graceful-shutdown-cont branch from 32c56d2 to b6440dc Compare April 26, 2023 15:06
@Kludex
Copy link
Member

Kludex commented Apr 26, 2023

Thanks @JonasKs 🙏

@Kludex Kludex changed the title Add --timeout-graceful-shutdown parameter Add --timeout-graceful-shutdown parameter Apr 26, 2023
@Kludex Kludex merged commit 2c0ea0a into encode:master Apr 26, 2023
@JonasKs JonasKs mentioned this pull request Apr 26, 2023
@JonasKs
Copy link
Contributor Author

JonasKs commented Apr 28, 2023

Hmm, didn't really intend for this to work well with --reload, but I can see why you would want it. I'll have to look further into it, but I won't be able to until the weekend. Please help investigate if you have time.

@JonasKs
Copy link
Contributor Author

JonasKs commented May 5, 2023

What happened with that comment, @Kludex ? I tried looking back to find what it was all about again..

@Kludex
Copy link
Member

Kludex commented May 5, 2023

I don't know. I don't delete my comments. Was it a comment from someone else?

@JonasKs
Copy link
Contributor Author

JonasKs commented May 5, 2023

Yeah, someone said it didn't work properly with --reload I believe. I'll test some more this weekend, I don't really remember 😅

@oliverhaas
Copy link

oliverhaas commented Jun 13, 2024

The parameter works fine with --reload, e.g.

uvicorn config.asgi:application --host 0.0.0.0 --reload --reload-include '*.html' --timeout-graceful-shutdown 0

Hopefully this is not too offtopic, but hopefully useful someone ends up here looking for information like I just did.

@JonasKs
Copy link
Contributor Author

JonasKs commented Jun 13, 2024

That's a good tip, should probably be documented. Potentially --reload should set it to 0 by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make force_exit configure-able from command line How do I cancel a streaming response on shutdown?
3 participants