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 delayed-shutdown #2414

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jakobhenningjensen
Copy link

Summary

I have seen several issues/question on e.g StackOverflow regarding "gracefull shutdown" in FastAPI/uvicorn, and I had the same issue. While we have the timeout_graceful_shutdown there is a a problem in e.g Kubernetes. When we are scaling number of pods/instances down, we are sending a SIGTERM to the pod, but the load-balancer keep sending requests until the health/ready does not respond status 200. Say the load-balancer pings health/ready every 5 seconds until it gets a non-200-status, then the app could still get requests for 5 seconds after the SIGTERM has been received meaning that all requests in that 5 sec interval would fail.

By adding a delayed shutdown we simply delay the shutdown of the app by a given number of seconds, while setting a flag in the serving app (uvicorn_shutdown_triggered). The app can then use this property to check, if a health-point should return 200 or something else to notify the load-balancer that the app is currently under shutdown, while still accepting requests. After the delayed_shutdown time has passed the app shuts down as it normally would.

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.

@mautini
Copy link

mautini commented Nov 11, 2024

This is exactly what I'm looking for. A workaround in k8s environment is to use PreStop hook but it would more convenient to have this option natively in unicorn.

However, I see that this MR is 3 months old. Any chance to get it accepted a some point? Is there anything I can do to help?

@Jakobhenningjensen
Copy link
Author

This is exactly what I'm looking for. A workaround in k8s environment is to use PreStop hook but it would more convenient to have this option natively in unicorn.

However, I see that this MR is 3 months old. Any chance to get it accepted a some point? Is there anything I can do to help?

@mautini That would be great! I'm currently struggling with the part on how to propagate that a shutdown is triggered, to the "app" served by uvicorn.
If it is a FastAPI app then we can just change it like so

        if self.config.shutdown_delay:
            logger.info(f"Shutting down in {self.config.shutdown_delay} seconds")
            self.config.app.uvicorn_shutdown_triggered = True
            await asyncio.sleep(self.config.shutdown_delay)

but since app could be a lot of stuff (where we aren't able to set a new attribute), we need some other variable that could be set to True.
One work around would be a simple class

class ShutdownTriggered:
    is_shutdown_triggered: bool = False

where we then modify the class-attribute is_shutdown_triggered. Then other users can import that class and the attribute should change accordingly during shutdown (and you can make your endpoint return some other code than 200 when is_shutdown_triggered == False).

It might not be the prettiest way of doing it, but I can't se any other way, since I'm not that deep into uvicorn.

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.

2 participants