Stop body_stream in case more_body=False

[Kludex]

Closes #2093

We need to check how this interacts with FastAPI: #1609 (comment)

I've tested this with the following application:

from asyncio import sleep
from typing import Any, Coroutine

from starlette.applications import Starlette
from starlette.background import BackgroundTasks
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint
from starlette.middleware.gzip import GZipMiddleware
from starlette.requests import Request
from starlette.responses import JSONResponse, Response, StreamingResponse
from starlette.routing import Route

n = 0


async def task() -> None:
    global n
    local = n
    print(f"Start {local}.")
    n += 1
    await sleep(3)
    print(f"Finish {local}.")


class CustomMiddleware(BaseHTTPMiddleware):
    async def dispatch(
        self, request: Request, call_next: RequestResponseEndpoint
    ) -> Coroutine[Any, Any, Response]:
        return await call_next(request)


async def homepage(request: Request) -> ...:
    tasks = BackgroundTasks()
    tasks.add_task(task)
    return JSONResponse(content={"potato": "potato"}, background=tasks)


def exc_stream(request: Request):
    return StreamingResponse(_generate_faulty_stream())


def _generate_faulty_stream():
    yield b"Ok"
    raise Exception("Faulty Stream")


def empty(request: Request):
    return Response(status_code=200)


app = Starlette(
    routes=[
        Route("/", endpoint=homepage),
        Route("/stream", endpoint=exc_stream),
        Route("/empty", endpoint=empty),
    ],
    middleware=[Middleware(GZipMiddleware), Middleware(CustomMiddleware), Middleware(GZipMiddleware)],
)

Run with uvicorn --http h11 main:app --reload.

I've used the following client:

from httpx import Client

with Client(base_url="http://localhost:8000") as client:
    print("Calling '/'")
    for n in range(5):
        client.get("/")

    print("Calling '/empty'")
    for n in range(5):
        client.get("/empty")

#     print("Calling '/stream'")
#     for n in range(5):
#         client.get("/stream")

Run with python client.py.

[Kludex]

I've tested against FastAPI main branch, and it looks like it doesn't break the dependency injection system, as the previous attempt.

@encode/maintainers Can someone review it, please? I want us to make sure that we don't break FastAPI. If you have a suggestion about how to test this, it would be appreciated as well.

[alex-oleshkevich]

That's ok. break closes the iterator just like if there are no more data.

[adriangb]

On fastapi==0.100.0 #1609 (comment) works correctly for me either with the version of starlette it ships with (0.27.X) nor 0.28.0. It also seems to work with this branch. I once again confirmed it's broken on fastapi==0.85.0. So it seems that at the very least this change does not break FastAPI in the same way as the previous one did.

As long as we can confirm via a test that this fixes the issues with background tasks this looks good to me 👍🏻

[Kludex]

I gave another round of manual test with FastAPI, and it works as expected - not breaking their dependency system. 🙏

[b4stien]

Hi, this PR regressed this issue, I've tested manually that commenting those two lines allowed app_exc not to be None and to be subsequently raised.

(I'm no expert in ASGI and starlette codebase, don't understand the root issue that this change was trying to fix and I'm then unable to propose a better fix...)