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

Clean-shutdown the server even in early shutdown case #2433

Conversation

kichanyurd
Copy link

@kichanyurd kichanyurd commented Aug 15, 2024

Summary

When running the following code (pytest):

import asyncio
from contextlib import asynccontextmanager
from random import random
from typing import AsyncIterator
from fastapi import FastAPI
from fastapi.responses import JSONResponse
import uvicorn


@asynccontextmanager
async def run_server(app: FastAPI) -> AsyncIterator[None]:
    config = uvicorn.Config(app=app, port=8089)
    server = uvicorn.Server(config)
    task = asyncio.create_task(server.serve())
    yield
    server.should_exit = True
    await task


def rng_app() -> FastAPI:
    app = FastAPI()

    @app.get("/random_number")
    async def get_random_number(max: int) -> JSONResponse:
        return JSONResponse({"number": int(random() * max)})

    return app


async def test_this() -> None:
    async with run_server(rng_app()):
        pass

I'm getting the following error:

Task was destroyed but it is pending!
task: <Task pending name='Task-3' coro=<LifespanOn.main() running at /workspaces/emcie/server/.venv/lib/python3.12/site-packages/uvicorn/lifespan/on.py:86> wait_for=<Future pending cb=[Task.task_wakeup()]>>
ERROR:    Traceback (most recent call last):
  File "/home/vscode/.pyenv/versions/3.12.4/lib/python3.12/asyncio/queues.py", line 158, in get
    await getter
GeneratorExit

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/workspaces/emcie/server/.venv/lib/python3.12/site-packages/starlette/routing.py", line 743, in lifespan
    await receive()
  File "/workspaces/emcie/server/.venv/lib/python3.12/site-packages/uvicorn/lifespan/on.py", line 137, in receive
    return await self.receive_queue.get()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vscode/.pyenv/versions/3.12.4/lib/python3.12/asyncio/queues.py", line 160, in get
    getter.cancel()  # Just in case getter is not done yet.
    ^^^^^^^^^^^^^^^
  File "/home/vscode/.pyenv/versions/3.12.4/lib/python3.12/asyncio/base_events.py", line 795, in call_soon
    self._check_closed()
  File "/home/vscode/.pyenv/versions/3.12.4/lib/python3.12/asyncio/base_events.py", line 541, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed

This PR fixes the issue by making sure that a graceful shutdown (including that of the lifespan class) is always awaited before returning from the serve() function.

Please note that I didn't add a test as this meant incorporating Starlette/FastAPI in the tests as a (heavy) dependency.

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.

@Kludex
Copy link
Member

Kludex commented Aug 17, 2024

How do you have so many thumbs up here?

@kichanyurd
Copy link
Author

@Kludex It's currently affecting our test suite so some people helped call attention to this :)

@Kludex
Copy link
Member

Kludex commented Aug 18, 2024

@Kludex It's currently affecting our test suite so some people helped call attention to this :)

I see. Maybe instead of wasting your coworkers time, you can sponsor me. :)

Also, why do you run uvicorn on your test suite?

@kichanyurd
Copy link
Author

@Kludex I'm already sponsoring them anyway :) But happy to make a contribution!

I'm running it as part of an E2E test suite for our client/server setup. We have a complex server CLI that internally runs uvicorn by importing the library, and in these tests we automatically verify the behavior between the server CLI and the client CLI.

@kichanyurd
Copy link
Author

Hey @Kludex, I reckon you're quite busy over there. Is there anything I can do to facilitate the process in some way?

@Kludex
Copy link
Member

Kludex commented Sep 3, 2024

Well, it looks like you just don't have an event loop fixture? fastapi/fastapi#5692 (comment)

Please create a discussion. This is not a bug.

@Kludex Kludex closed this Sep 3, 2024
@kichanyurd
Copy link
Author

Well, it looks like you just don't have an event loop fixture? fastapi/fastapi#5692 (comment)

Please create a discussion. This is not a bug.

@Kludex we actually have an event loop running, and all of our tests are async tests which are using it. The uvicorn server does run and function correctly, aside from the shutdown issue, which it seems is due to not awaiting on all started tasks before the process is finished, leading to asyncio errors at the end of the process's lifetime.

Also attaching a link here to the discussion I previously opened on the topic: #2434

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