📝 Docs preview for commit e24e016 at: https://63851d6f5737095d3317664e--fastapi.netlify.app

This means more resource optimised DB connections?🤔

(just wondering)

Not really. It just means that sync context manager's __enter__ and __exit__ run in the same thread. In fact this might make things worse in some cases because the thread is checked out from the thread pool for the entirety of the context managers life. But I think it also eliminates a whole class of intermittent hard to reason about or figure out bugs.

Make sense, better house-keeping!

Thanks for your explanation btw! 😅

📝 Docs preview for commit e45a801 at: https://63852343acf33a6f2fa9c78e--fastapi.netlify.app

marked as ready, cc @tiangolo

@kglee0 or @alexted mind trying out this branch? It's a shot in the dark but worth at least confirming the null hypothesis.

📝 Docs preview for commit ac93694 at: https://638663ca7c5147091cf84317--fastapi.netlify.app

📝 Docs preview for commit 6ba173b at: https://638a9f62bb6c0b319b2736fb--fastapi.netlify.app

The Issue here

Awesome! Did you get to test this branch?

Awesome! Did you get to test this branch?

Yeah, it didn't have any effect.

📝 Docs preview for commit 5ac3f62 at: https://63a1f7af8c940416a38c635d--fastapi.netlify.app

📝 Docs preview for commit 04347ed at: https://63a4d7eaeccde823d1092dc0--fastapi.netlify.app

Is this pr possible to merge sometime soon? Coming here from this thread: MagicStack/asyncpg#978

For visibility: #8424 (comment)

The test from this PR doesn't fail with current FastAPI version (0.116.0):

import threading
from contextvars import ContextVar

from fastapi import Depends, FastAPI
from fastapi.testclient import TestClient

ctx: ContextVar[str] = ContextVar("ctx")

async def context_same_thread():
    # test both a real world use case and a
    # simpler synthetic check
    ctx.set("foo")
    ident = threading.get_ident()
    yield
    assert ctx.get() == "foo"
    assert ident == threading.get_ident()

app = FastAPI()

@app.get("/check_cm_same_thread")
async def check_cm_same_thread(_=Depends(context_same_thread)):
    return None

def test_sync_runs_in_single_thread():
    client = TestClient(app)
    response = client.get("/check_cm_same_thread")
    response.raise_for_status()

@adriangb, could you please double-check? Seems that it has been fixed.

UPD: shouldn't context_same_thread be sync?

As this PR has been waiting for the original user for a while but seems to be inactive, it's now going to be closed. But if there's anyone interested, feel free to create a new PR.

It's unfair to expect users to reply when review takes so long.

Isn't there a less aggressive strategy? Like pinging the user again a couple of weeks?

It's unfair to expect users to reply when review takes so long.
Isn't there a less aggressive strategy? Like pinging the user again a couple of weeks?

@Kludex, I agree we should change this.
We will update the script to add more explanations of how we work on external PRs and why we do it this way, also we will add a reminder 1 week before closing PR.

What should we do with this PR now?
@adriangb, are you still interested in this PR?
Could you please take a look at my comment?

This pull request has a merge conflict that needs to be resolved.

I'm realizing that this was related to a change done a while ago with dependencies with yield, this was solved in 0.74.0 (https://fastapi.tiangolo.com/release-notes/#0740)

The exit code after yield in a dependency was being executed in a different async context (contextvars context), so the value of contextvars was different than the original.

This was because custom middlewares would internally create an AnyIO task group, which creates a new context.

That logic was then refactored once again in 0.106.0 (https://fastapi.tiangolo.com/release-notes/#01060).

And is now refactored again here 😅 : #14099 , which will be released in version 0.118.0

But the most important point is, a test for this use case was added (in the first PR in the version described above) here: https://github.com/fastapi/fastapi/blob/master/tests/test_dependency_contextvars.py

So that should ensure this is handled/fixed and stays handled and fixed. 🚀

Given that, I'll now close this one, but if anyone faces a similar problem again, please create a new discussion with an example to replicate it so we can take a look. 🤓

Thanks all! 🍰