Do not send empty data to StreamingResponse on BaseHTTPMiddleware

[Kludex]

• This seems more correct than Set more_body to False when chunk is b"" #1608.
• Alternative to GZIP content length mismatch fix #1579
• Closes Gzip Middleware content-length is incorrect #803 not really related.

[Kludex]

@hiranp @abersheeran Can you confirm that the issue is solved with this PR? 🙏

[Kludex]

Thanks @abersheeran and @adriangb :)

[MrSalman333]

Hello,

this PR breaks down stream FastApi >_<

the tare down in the decencies in FastApi doesn't work as expected after it .

[adriangb]

Do you have a minimal reproducible example? Keep in mind that FastAPI uses a pinned Starlette version.

[MrSalman333]

yah , I'm typing a complete example now ,,,

FastAPI just updated Starlette version this is why we found the issue ,,

EDIT:

import logging

from fastapi import Depends, FastAPI
from fastapi.middleware.base import BaseHTTPMiddleware
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware


class CustomMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request, call_next):
        return await call_next(request)


app = FastAPI(middleware=[Middleware(CustomMiddleware)])


def dependency_a():
    yield "A"
    logging.warning("WILL NOT RUN")


def dependency_b():
    yield "B"
    logging.warning("WILL NOT RUN")


def dependency_c():
    yield "C"
    logging.warning("WILL RUN")


@app.get(
    "/issue",
    dependencies=[
        Depends(dependency_a),
    ],
)
def show_dependencies_issue(
    b=Depends(dependency_b),
    c=Depends(dependency_c),
):
    return {"status": "ok"}

ok so if I remove the CustomMiddleware from the app. all warning messages will log.
but with the code above ,, only the message with "WILL RUN" will run ,, because it is the last decencies

this issue disappear if I revert this change.
but I'm sure it will cause other issues,

[MrSalman333]

example added

[adriangb]

Try this:

import logging

import anyio
import httpx
from fastapi import Depends, FastAPI, Response
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.middleware import Middleware


class CustomMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request, call_next):
        return await call_next(request)


app = FastAPI(middleware=[Middleware(CustomMiddleware)])


def dependency_a(response: Response):
    try:
        yield "A"
    finally:
        logging.warning("A")


def dependency_b(response: Response):
    try:
        yield "B"
    finally:
        logging.warning("B")


def dependency_c(response: Response):
    try:
        yield "C"
    finally:
        logging.warning("C")


@app.get(
    "/issue",
    dependencies=[
        Depends(dependency_a),
    ],
)
def show_dependencies_issue(
    b=Depends(dependency_b),
    c=Depends(dependency_c),
):
    return {"status": "ok"}


async def main() -> None:
    async with httpx.AsyncClient(app=app, base_url="http://example.com") as client:
        await client.get("/issue")

anyio.run(main)

What version of FastAPI does your example work in?

[Kludex]

I can reproduce the issue.

I've isolated the commit, it's confirmed. This changes the behavior.

[MrSalman333]

lates version[0.86.0] "the current master"

but the issue should show up on > 0.85, because they updated Starlette version to [0.20.4]

[Kludex]

Let's understand if this commit has an issue or if the problem is the FastAPI assumptions around dependencies.

I'll be available to check this tonight (Amsterdam time), jfyk.

[MrSalman333]

take a look at this file from FastAPI I think u can get there working assumptions

https://github.com/tiangolo/fastapi/blob/master/fastapi/applications.py

check their override of build_middleware_stack and the comment in it

[MrSalman333]

I'll open a PR on FastAPI that fixes the issue from there side ,, but it will change the behavior of contextvars on FastAPI so not sure if it will get accepted,

just letting you know

[Kludex]

Try this:

import logging

import anyio
import httpx
from fastapi import Depends, FastAPI, Response
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.middleware import Middleware


class CustomMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request, call_next):
        return await call_next(request)


app = FastAPI(middleware=[Middleware(CustomMiddleware)])


def dependency_a(response: Response):
    try:
        yield "A"
    finally:
        logging.warning("A")


def dependency_b(response: Response):
    try:
        yield "B"
    finally:
        logging.warning("B")


def dependency_c(response: Response):
    try:
        yield "C"
    finally:
        logging.warning("C")


@app.get(
    "/issue",
    dependencies=[
        Depends(dependency_a),
    ],
)
def show_dependencies_issue(
    b=Depends(dependency_b),
    c=Depends(dependency_c),
):
    return {"status": "ok"}


async def main() -> None:
    async with httpx.AsyncClient(app=app, base_url="http://example.com") as client:
        await client.get("/issue")

anyio.run(main)

What version of FastAPI does your example work in?

Hmmm... This example shows that the dependency is not teardown-ed. Using this code with uvicorn, you can see the logs on SIGINT.

[adriangb]

Hmmm... This example shows that the dependency is not teardown-ed. Using this code with uvicorn, you can see the logs on SIGINT.

When I run this on fastapi==0.86.0 and starlette==0.20.4 I see all 3 logs emitted. With the same versions the original example does indeed give me only WARNING:root:WILL RUN.

[Kludex]

Hmmm... This example shows that the dependency is not teardown-ed. Using this code with uvicorn, you can see the logs on SIGINT.

When I run this on fastapi==0.86.0 and starlette==0.20.4 I see all 3 logs emitted. With the same versions the original example does indeed give me only WARNING:root:WILL RUN.

I didn't check this scenario. I'll check later on.

[MrSalman333]

Hmmm... This example shows that the dependency is not teardown-ed. Using this code with uvicorn, you can see the logs on SIGINT.

When I run this on fastapi==0.86.0 and starlette==0.20.4 I see all 3 logs emitted. With the same versions the original example does indeed give me only WARNING:root:WILL RUN.

running this with uvicorn, still shows the issue

in our project the test client will not show the same behavior, in the tests this works with no issue

so using try ... finally with AsyncClient will not show this issue

[Kludex]

The problem is only with def dependencies.

I think we know where the issue is... I'll check.

[Kludex]

From AsyncExitStackMiddleware:

Traceback (most recent call last):
  File "/home/marcelo/Development/fork/fastapi/fastapi/middleware/asyncexitstack.py", line 18, in __call__
    await self.app(scope, receive, send)
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/starlette/routing.py", line 680, in __call__
    await route.handle(scope, receive, send)
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/starlette/routing.py", line 275, in handle
    await self.app(scope, receive, send)
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/starlette/routing.py", line 68, in app
    await response(scope, receive, send)
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/starlette/responses.py", line 167, in __call__
    await send({"type": "http.response.body", "body": self.body})
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/starlette/middleware/exceptions.py", line 61, in sender
    await send(message)
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/anyio/streams/memory.py", line 215, in send
    await send_event.wait()
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/anyio/_backends/_asyncio.py", line 1843, in wait
    await checkpoint()
  File "/home/marcelo/.pyenv/versions/fastapi3.7/lib/python3.7/site-packages/anyio/_backends/_asyncio.py", line 515, in checkpoint
    await sleep(0)
  File "/home/marcelo/.pyenv/versions/3.7-dev/lib/python3.7/asyncio/tasks.py", line 585, in sleep
    await __sleep0()
  File "/home/marcelo/.pyenv/versions/3.7-dev/lib/python3.7/asyncio/tasks.py", line 579, in __sleep0
    yield
concurrent.futures._base.CancelledError