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

Context managers in Depends are broken after 0.106 #11143

Open
9 tasks done
Kludex opened this issue Feb 14, 2024 Discussed in #11107 · 17 comments
Open
9 tasks done

Context managers in Depends are broken after 0.106 #11143

Kludex opened this issue Feb 14, 2024 Discussed in #11107 · 17 comments
Labels
question Question or problem

Comments

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Feb 14, 2024

Discussed in #11107

Originally posted by FeeeeK February 7, 2024

First Check

  • I added a very descriptive title here.
  • I used the GitHub search to find a similar question and didn't find it.
  • I searched the FastAPI documentation, with the integrated search.
  • I already searched in Google "How to X in FastAPI" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to FastAPI but to Pydantic.
  • I already checked if it is not related to FastAPI but to Swagger UI.
  • I already checked if it is not related to FastAPI but to ReDoc.

Commit to Help

  • I commit to help with one of those options 👆

Example Code

from fastapi import Depends, FastAPI, Request

app = FastAPI()


class Session:
    def __init__(self):
        print("creating session")

    async def __aenter__(self):
        print("opening session")
        return self

    async def __aexit__(self, exc_type, exc, tb):
        print("closing session")

    async def commit(self):
        print("committing session")

    async def rollback(self):
        print("rolling back session")


@app.middleware("http")
async def commit_session(request: Request, call_next):
    # minimalistic middleware for example, my code uses ASGI middleware
    response = await call_next(request)
    db_session = request.scope.get("db_session")
    if not db_session:
        return response

    if response.status_code // 200 != 1:
        await db_session.rollback()
    else:
        await db_session.commit()

    return response


async def get_db_session(request: Request):
    async with Session() as session:
        request.scope["db_session"] = session
        yield session


@app.get("/")
async def root(session: Session = Depends(get_db_session)):
    return {"message": "Hello World"}


# Pre 0.106 behaviour:

# creating session
# opening session
# committing session
# closing session

# Post 0.106 behaviour:
# creating session
# opening session
# closing session
# committing session

# The session is not committed, because it's closed before the middleware is called.

Description

Before 0.106, Depends execution after yield was after middlewares, which allowed to access resources created for a route (e.g. sessions) and do something with them depending on the response (which cannot be done with Depends), but after 0.106, the behavior has changed and this feature is no longer available. The documentation only talks about background tasks, but not a word about middlewares. Was this behavior change intentional?

Operating System

Windows

Operating System Details

No response

FastAPI Version

0.109.2

Pydantic Version

2.4.2

Python Version

3.11.1

Additional Context

No response

@Kludex Kludex added the question Question or problem label Feb 14, 2024
@falkben
Copy link
Sponsor Contributor

falkben commented Feb 14, 2024

I commented in the Discussion that StreamingResponse's no longer work with Depends resources. Since this is a pretty large change in behavior, and wasn't called out in the release notes, I wonder if this is intended. Should this be a separate Issue?

@Kludex
Copy link
Sponsor Collaborator Author

Kludex commented Feb 14, 2024

I commented in the Discussion that StreamingResponse's no longer work with Depends resources. Since this is a pretty large change in behavior, and wasn't called out in the release notes, I wonder if this is intended. Should this be a separate Issue?

No. The only reason I created this issue is because of your comment. 🙏

@Kludex
Copy link
Sponsor Collaborator Author

Kludex commented Feb 22, 2024

Does this comment by @tiangolo answers this issue: #11177 (reply in thread) ?

@falkben
Copy link
Sponsor Contributor

falkben commented Feb 22, 2024

I might be misinterpreting but I don't think so? Seems like it was intended to disable using resources in background tasks. But was it intended to also disable using resources in path operators with StreamingResponses?

@mortalisk
Copy link

It seems this is what breaks our streaming endpoints, the lack of which kills throughput making everything slow.

@mortalisk
Copy link

A workaround for now is to use our connection pool directly in each endpoint doing streaming. Annoying, but works.
Doing something like this for a psycopg pool:

@router.get("/stuff", response_model=list[Stuff])
async def get_all_stuff(pool: PoolDep):
    connection = await pool.getconn()

    async def commit_and_put_in_pool():
        await connection.commit()
        await pool.putconn(connection)

    return StreamingResponse(
            stream_stuff(connection), media_type="application/json", background=commit_and_put_in_pool
    )

This seems to work for us. But is it correct? Will the background task always run after the streaming is done? Or can we end up in a situation where it is run in the middle of streaming?

@brian-goo
Copy link

brian-goo commented Mar 14, 2024

Our streaming endpoints are broken probably due to this.
So it seems that there is no good workaround as long as a db session is managed with Depends?

@scriptator
Copy link

scriptator commented Mar 21, 2024

Our streaming endpoints are broken probably due to this. So it seems that there is no good workaround as long as a db session is managed with Depends?

The release notes suggest the following:

If you used to rely on this behavior, now you should create the resources for background tasks inside the background task itself, and use internally only data that doesn't depend on the resources of dependencies with yield.

For example, instead of using the same database session, you would create a new database session inside of the background task, and you would obtain the objects from the database using this new session. And then instead of passing the object from the database as a parameter to the background task function, you would pass the ID of that object and then obtain the object again inside the background task function.

i.e. just don't use Depends for obtaining your db session for background tasks any more but obtain it within the background task

@falkben
Copy link
Sponsor Contributor

falkben commented Mar 21, 2024

@scriptator we're not talking about background tasks here.

@scriptator
Copy link

@scriptator we're not talking about background tasks here.

I was replying to @brian-goo who talked about streaming responses which behave like background tasks in this regard. I had a problem because I used a db session in a streaming response that I obtained with Depends and it was fixed by following the advice for background tasks: initiate your resources within the background task (or generator function in the case of streaming responses).

But you are right if you meant that this does not contribute to the original problem from the issue.

@falkben
Copy link
Sponsor Contributor

falkben commented Mar 21, 2024

I don't think streaming responses should be considered similar to background tasks. They are part of the request/response cycle. I think their behavior in this case being similar to background tasks not being able to use dependency injection in the path operators is likely a bug.

@mortalisk
Copy link

mortalisk commented Apr 11, 2024

Any progress on this? Our workaround is not working as expected. It seems the background task is not guaranteed to run, and thus our connection pool is failing when ever there is any error, essentially taking down the entire program, and we must restart.

This basically renders streaming useless for us.

Is there something else we can do to make it work?

@hasansustcse13
Copy link

hasansustcse13 commented Apr 16, 2024

I am facing the same problem. Does anybody know any workaround for this problem?
@tiangolo
#11444

@falkben
Copy link
Sponsor Contributor

falkben commented Apr 16, 2024

For now, just staying on version <0.106.

@mortalisk
Copy link

Does <0.106 work on python 3.12? Because 0.109 says "Add support for Python 3.12."

@btemplep
Copy link

+1 for this issue. It breaks request context management for stream proxies.

ie I have a Depends parameter for connecting to an external streaming API. I start a stream from an external API in the route. I need the initial response from the external API to contribute to the StreamResponse return. Then I need that same connection to stream from my streaming generator.

The work around in my case is to use a global client for the external streaming API so that cleanup is not called on the return of StreamingResponse. As well as managing the context with try/finally blocks in the route and streaming generator. Then more context managers in the streaming generator for other Depends that die when the route returns.

@libe
Copy link

libe commented May 6, 2024

@btemplep could you provide a code example for your workaround?

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

No branches or pull requests

8 participants