uvicorn icon indicating copy to clipboard operation
uvicorn copied to clipboard

uvicorn eats SIGINTs, does not propagate exceptions

Open fastily opened this issue 3 years ago • 18 comments

The following snippet cannot be killed with a SIGINT (ctrl+c):

import asyncio

from starlette.applications import Starlette
from uvicorn import Config, Server

async def web_ui():
    await Server(Config(Starlette())).serve()

async def task():
    await asyncio.sleep(100000000000000)

async def main():
    await asyncio.gather(web_ui(), task())

if __name__ == "__main__":
    asyncio.run(main())

It appears that uvicorn is eating SIGINTs and does not propagate the KeyboardInterrupt and/or Cancelled exceptions. Thanks for having a look.

[!IMPORTANT]

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar

fastily avatar Jul 18 '22 09:07 fastily

I saw this getting flagged by SonarQube.

#  h11_impl.py line 401
 async def run_asgi(self, app: "ASGI3Application") -> None:
        try:
            result = await app(self.scope, self.receive, self.send)
        except BaseException as exc:
            msg = "Exception in ASGI application\n"
            self.logger.error(msg, exc_info=exc)
            if not self.response_started:
                await self.send_500_response()
            else:
                self.transport.close()

There are multiple instances of it in other files as well.

Sonar:

"SystemExit" should be re-raised
 
Code smell
 
Critical
python:S5754

iudeen avatar Jul 25 '22 10:07 iudeen

I'm having trouble with the '--reload' option inside docker. Nothing reloads. I suspect it might be related to this. I've tried running uvicorn via the watchfiles cli, and SIGINTs get swallowed.

circlingthesun avatar Jul 27 '22 10:07 circlingthesun

Here's a very hacky workaround

from uvicorn import Server

class MyCustomUvicornServer(Server):
    def install_signal_handlers(self):
        pass

fastily avatar Jul 27 '22 20:07 fastily

Well, for my problem I've uninstalled watchfiles so uvicorn falls back to polling which works.

circlingthesun avatar Jul 27 '22 22:07 circlingthesun

This has turned out to be a real problem for running an optional uvicorn based server inside an existing asyncio application. Since Server.serve finishes gracefully and the original signal handlers are never restored there is no way to ^C the application itself.

It looks okay'ish for Server.serve to capture signals for graceful shutdown, but it should at least restore the original signal handlers and ideally also reproduce the original behaviour as well. From the looks of it, Server.serve should raise KeyboardInterrupt when done and only Server.run should suppress it.

maxfischer2781 avatar Jul 28 '22 12:07 maxfischer2781

It looks okay'ish for Server.serve to capture signals for graceful shutdown, but it should at least restore the original signal handlers and ideally also reproduce the original behaviour as well. From the looks of it, Server.serve should raise KeyboardInterrupt when done and only Server.run should suppress it.

Yes. Running into this exact scenario at the moment: I'm wrapping Server.serve in an asyncio task (to gather with other tasks) and the signal swallowing makes it impossible for me to catch a server shutdown to properly cancel the other tasks.

bachya avatar Jul 29 '22 03:07 bachya

This has caused us some weird problems at Prefect as well. Most recently, I've worked around it with funky usage of a cancel scope but this took a lot of trial and error to get working at all. Would it be feasible to merge something like #1600 or is there interest in a different serve API that is context managed and doesn't need to use signals for shutdown?

zanieb avatar Oct 26 '22 21:10 zanieb

^ courtesy ping for @Kludex , who seems to be the most active maintainer

fastily avatar Oct 27 '22 04:10 fastily

I have the same problem, I wonder if UviCorn could respect previously added signal handlers and combine them, I have a draft proposal in #1768 . This is just a draft to spark discussion

JoanFM avatar Nov 18 '22 09:11 JoanFM

What's the problem with the following?

import asyncio

from starlette.applications import Starlette
from uvicorn import Config, Server


async def web_ui():
    print("Inside web_ui")
    await Server(Config(Starlette())).serve()


async def sleep():
    try:
        print("Inside task")
        await asyncio.sleep(100000000000000)
    except asyncio.CancelledError:
        print("I can handle the cancellation here.")


async def main():
    task = asyncio.create_task(sleep())
    web_ui_task = asyncio.create_task(web_ui())
    await asyncio.wait([web_ui_task, task], return_when=asyncio.FIRST_COMPLETED)


if __name__ == "__main__":
    asyncio.run(main())
    print("It finished!")

Kludex avatar Jan 17 '23 14:01 Kludex

@Kludex Even when adding such code to manually handle the exit, the exit code is wrong for any naive handling. The program shown exits with 0; it should propagate the SIGINT if killed by that, though. A proper exit is via KeyboardInterrupt iff the application was killed by SIGINT.

That needs quite some extra scaffolding to do right.

maxfischer2781 avatar Jan 17 '23 14:01 maxfischer2781

I'm trying to assess the issue.

It appears that uvicorn is eating SIGINTs and does not propagate the KeyboardInterrupt and/or Cancelled exceptions.

With the above, the CancelledError happens.

@Kludex Even when adding such code to manually handle the exit, the exit code is wrong for any naive handling. The program shown exits with 0; it should propagate the SIGINT if killed by that, though. A proper exit is via KeyboardInterrupt iff the application was killed by SIGINT.

That needs quite some extra scaffolding to do right.

What is the issue you are trying to solve? I mean real case scenario.

Kludex avatar Jan 17 '23 15:01 Kludex

Hey @Kludex — a use case I have is to run an API server temporarily. For example, to facilitate a browser-based login experience:

  1. The user runs prefect cloud login
  2. We start an API server from our CLI
  3. We open the browser to perform login
  4. The browser then sends credentials to the API server
  5. The API server is shutdown

Right now, there are a lot of problems with using Uvicorn for a feature like this:

  1. When the server is misconfigured, serve will raise a SystemExit exception; this makes displaying errors to users complex and a SystemExit does not make sense when Uvicorn is not the owner of the process
  2. A SIGINT must be raised to stop the server; raising a SIGINT in the main thread to stop a single async task does not make sense
  3. After the server exits, all future keyboard interrupts will have no effect; this breaks our CLI user experience and could have more detrimental effects in an application

Does that make sense? I definitely could be doing it wrong, but it feels like the serve interface is designed to take control of a process rather than to be a first-class way to run a server programmatically. It'd be great to:

  1. Use a context manager to start and stop the server
  2. Only handle signals during the server runtime, propagate any received signal after handling
  3. Avoid assuming process level ownership (like calling sys.exit) when not started via the CLI entrypoint

zanieb avatar Jan 17 '23 18:01 zanieb

With the above, the CancelledError happens.

This is not sufficient for an asyncio application. KeyboardInterrupt is needed to signal to the event loop to shut down all tasks, not just those manually tied to the uvicorn server; an application is impossible to shut down gracefully otherwise. It is also needed to signal to the orchestration of the program (be this a shell, systemd, or similar) that shutdown occurred properly.

What is the issue you are trying to solve? I mean real case scenario.

I'm not sure what kind of answer you are expecting to this. "run uvicorn as part of a larger async application" is a realistic use-case in my book. This requires being able to shut down the application using standard means; right now uvicorn prevents this (at least without extensive workarounds).

In our specific case, we use uvicorn to implement an (optional) REST API of a highly concurrent resource management application. Mishandling SIGINT means that the application could not properly shut down and needed a hard kill, leaking other resources.

maxfischer2781 avatar Jan 17 '23 19:01 maxfischer2781

Hi all,

To chime in with another example.

I have a fairly trivial application where in a websocket handler, I'm using an async queue to push messages to clients. On shutdown, I'm using an event and a None message to signal we are closing down. But the fastapi shutdown handler is never applied and I cannot trigger these sentinel messages. It's ina deadlock position as far as I can tell.

Lawouach avatar Feb 06 '23 16:02 Lawouach

I don't know if this could help but this is my workaround for my use case:

import signal
from types import FrameType

from fastapi import FastAPI 

app = FastAPI()


@app.on_event("startup")
async def startup_event() -> None:
    default_sigint_handler = signal.getsignal(signal.SIGINT)
    def terminate_now(signum: int, frame: FrameType = None):
        # do whatever you need to unblock your own tasks

        default_sigint_handler(signum, frame)
    signal.signal(signal.SIGINT, terminate_now)

Lawouach avatar Feb 06 '23 19:02 Lawouach

You can just override install_signal_handlers function:

# FastAPI Uvicorn override
class Server(uvicorn.Server):

    # Override
    def install_signal_handlers(self) -> None:

        # Do nothing
        pass

And then when you catch interrupt elsewhere you can just signal server to stop:

self.server.should_exit = True

digitalik avatar Jun 23 '23 15:06 digitalik

You can just override install_signal_handlers function:

# FastAPI Uvicorn override
class Server(uvicorn.Server):

    # Override
    def install_signal_handlers(self) -> None:

        # Do nothing
        pass

And then when you catch interrupt elsewhere you can just signal server to stop:

self.server.should_exit = True

Nice! one more option is to override the handle_exit:

on_exit = lambda: logging.getLogger(__name__).info("Exiting...") 
class Server(uvicorn.Server):
        """Override uvicorn.Server to handle exit."""

        def handle_exit(self, sig, frame):
            # type: (int, Optional[FrameType]) -> None
            """Handle exit."""
            on_exit()
            super().handle_exit(sig=sig, frame=frame)

lazToum avatar Jul 27 '23 20:07 lazToum