Prevent BaseHTTPMiddleware from hiding errors of StreamingResponse

[o-fedorov]

The issue is observed when:

• an application uses a BaseHTTPMiddleware subclass;
• it provides StreamingResponse;
• there is an exception raised inside the generator wrapped with StreamingResponse.

Prior to v0.17.0 the exception was raised and the traceback was printed to the server logs. Now it is silently ignored.

Edit by @Kludex : Closes #1433

[Kludex]

Alternative to #1452

Would you mind adding this test?

def test_exception_on_mounted_apps(test_client_factory):
    sub_app = Starlette(routes=[Route("/", exc)])
    app.mount("/sub", sub_app)

    client = test_client_factory(app)
    with pytest.raises(Exception) as ctx:
        client.get("/sub/")
    assert str(ctx.value) == "Exc"

[o-fedorov]

Alternative to #1452

Would you mind adding this test?

def test_exception_on_mounted_apps(test_client_factory):
    sub_app = Starlette(routes=[Route("/", exc)])
    app.mount("/sub", sub_app)

    client = test_client_factory(app)
    with pytest.raises(Exception) as ctx:
        client.get("/sub/")
    assert str(ctx.value) == "Exc"

The test is added

[Kludex]

I don't think this work if the Response object from the middleware is ignored, as below:

from starlette.middleware.base import BaseHTTPMiddleware
from starlette.responses import Response

class CustomHeaderMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request, call_next):
        await call_next(request)
        return Response("Do you see an exception now?")

middleware = [
    Middleware(CustomHeaderMiddleware)
]

app = Starlette(routes=routes, middleware=middleware)

Can you confirm? Using the same StreamingResponse endpoint, you created on your test with this middleware.

#1452 should be able to solve this.

[o-fedorov]

Sorry, both this approach and #1452 seems to not solving the issue if the Response object from the middleware is ignored

[Kludex]

Sorry, both this approach and #1452 seems to not solving the issue if the Response object from the middleware is ignored

My bad... That exception is not even raised! 😆

In any case, I like this PR more than the one I created because it brings the exception conditional closer to where it can be retrieved.

[Kludex]

More context about this PR can be found on #1452 and #1433.

The issue here is that we check if the endpoint (or app) raised an exception only if http.response.start is not sent, but StreamingResponse (and mounted apps) pass that conditional, and we can have an exception after that point.

[Kludex]

Thanks for the PR @o-fedorov ! :)