tiangolo / fastapi

FastAPI framework, high performance, easy to learn, fast to code, ready for production
https://fastapi.tiangolo.com/
MIT License
73.66k stars 6.21k forks source link

Potential footgun when using custom `Response(background=<Task()>)` in conjunction with injected `BackgroundTasks` - the custom response overwrites the tasks #11215

Open Kludex opened 4 months ago

Kludex commented 4 months ago

Discussed in https://github.com/tiangolo/fastapi/discussions/11214

Originally posted by **netanel-haber** February 28, 2024 ### First Check - [X] I added a very descriptive title here. - [X] I used the GitHub search to find a similar question and didn't find it. - [X] I searched the FastAPI documentation, with the integrated search. - [X] I already searched in Google "How to X in FastAPI" and didn't find any information. - [X] I already read and followed all the tutorial in the docs and didn't find an answer. - [X] I already checked if it is not related to FastAPI but to [Pydantic](https://github.com/pydantic/pydantic). - [X] I already checked if it is not related to FastAPI but to [Swagger UI](https://github.com/swagger-api/swagger-ui). - [X] I already checked if it is not related to FastAPI but to [ReDoc](https://github.com/Redocly/redoc). ### Commit to Help - [X] I commit to help with one of those options 👆 ### Example Code ```python from fastapi import FastAPI, BackgroundTasks from starlette.responses import Response, BackgroundTask import uvicorn app = FastAPI() @app.get("/") async def endpoint(tasks: BackgroundTasks): tasks.add_task(lambda: print("This won't be printed")) return Response(content="Custom response", background=BackgroundTask(lambda: print("Only this will be printed"))) uvicorn.run(app) ``` ### Description Hey! Thanks for the best framework in Python and generally ❤️ Basically, passing a background task to a custom response overwrites any other tasks added to the injected `BackgroundTasks`. I'm not sure this is considered a bug, but I think it's confusing and can at least be clarified in the [Return a Response Directly](https://fastapi.tiangolo.com/advanced/response-directly/) docs, for example. Other solutions can be actually changing this behavior (I guess that's a breaking change), or printing a warning/throwing [throwing is also a breaking change] if both mechanisms are used at once. But I can't imagine a user desiring this behavior. To be clear, this tripped us up when we couldn't figure out why some of our tasks were being swallowed. I'd be happy to work on either solution, obviously adding a docs pr would be the easiest first step. ### Operating System macOS ### Operating System Details _No response_ ### FastAPI Version 0.97.0 ### Pydantic Version 1.10.7 ### Python Version 3.9 ### Additional Context _No response_
Kludex commented 4 months ago

I've created the issue because I think we can do better than just ignoring those background tasks. Maybe we can add them to the returned Response object?

netanel-haber commented 4 months ago

That's what I alluded to when I mentioned:

...actually changing this behavior

Meaning, somehow making it so both the injected tasks, and the passed background are executed as expected. For example, is it possible to append thebackground task to the injected tasks? Does FastAPI/Starlette provide a way to get the injections for a given request? Is that available to the Response class (and inheritors like StreamingResponse)?

This is a breaking change because existing apps might find that behavior suddenly changes (=tasks that weren't running before due to this overwrite suddenly run) - like I mentioned, this "breaking change" is really fixing a behavior that doesn't really make sense. But it kind of depends on the repo's policy, I think.

I'd be happy to contribute a pr for this, either way.

Tintean-Devops commented 4 months ago

Adding this because it seems related. It is certainly a source of an incredibly obscure bug that had me scratching my head for days. There's nothing in FastAPI that stops you caching a frequently used Response in a variable:

RESPONSE = HTMLResponse('ok')

If you return such a response from an endpoint that adds a background task you get a very obscure bug. This particular task takes a string argument. The first time you call the endpoint everything seems ok. The second time you call it, the background task is invoked with the same argument value as first time. I presume this is because of the same smoke and mirrors noted by @netanel-haber, whereby the Response is actually used to stash the background task context.

Here's a fully worked example:

# wtest.py
import uvicorn
from fastapi import FastAPI, BackgroundTasks
from fastapi.responses import HTMLResponse
import random
import string

RESPONSE = HTMLResponse('ok')

app = FastAPI()

def random_string():
    letters = string.ascii_lowercase
    return ''.join(random.choice(letters) for i in range(20))

async def background_task(arg: str) -> None:
    print(f"{arg=}")

@app.get("/api", response_class=HTMLResponse)
async def api(bk: BackgroundTasks) -> str:
    param = random_string()
    print(f"{param=}")
    bk.add_task(background_task, param)
    return RESPONSE

if __name__ == "__main__":
    uvicorn.run(
        "wtest:app",
        host="0.0.0.0",
        port=80,
        log_level="info",
        reload=True,
    )

Here's the output when you invoke /api twice. param is the parameter passed to the background task and arg is the argument as seen by the task. They should match up. Second time round, they don't:

INFO:     Application startup complete.
param='cmvlzpyrzyzddhmwikri'
INFO:     127.0.0.1:53216 - "GET /api HTTP/1.1" 200 OK
arg='cmvlzpyrzyzddhmwikri'
param='quomuzuwsbjvfqsjamrx'
INFO:     127.0.0.1:53216 - "GET /api HTTP/1.1" 200 OK
arg='cmvlzpyrzyzddhmwikri'```