Raise exception from background task on BaseHTTPMiddleware

[Kludex]

• Fixes middleware causes exceptions to not be raised/handled silently (back again) #2625

How to reproduce:

import asyncio
from typing import Any

from starlette.applications import Starlette
from starlette.background import BackgroundTask
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.requests import Request
from starlette.responses import JSONResponse
from starlette.routing import Route


class CustomHeaderMiddleware(BaseHTTPMiddleware):
    async def dispatch(self, request: Request, call_next: Any):
        response = await call_next(request)
        return response


async def bg_task():
    await asyncio.sleep(0.1)
    raise ValueError("TEST")


def homepage(request: Request):
    task = BackgroundTask(bg_task)
    message = {"status": "Ok"}
    return JSONResponse(message, background=task)


routes = [Route("/", homepage)]

middleware = [Middleware(CustomHeaderMiddleware)]

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

[Kludex]

@graingert would you mind reviewing this? 🙏

[graingert]

ah I broke it

[graingert]

ok back working again

[Kludex]

Why was this added?

[graingert]

it's subtle but it's needed to get test_custom_middleware to pass

the reason is app_exc shouldn't be subject to collapse_excgroups (it's outside of with recv_stream, send_stream, collapse_excgroups():)- eg it's from the user's code so might have use a TG or Nursery and therefore legitimately wrapped in a EG

however that means if something raises an exception from StreamingResponse they're not really expecting an (optional!) background task to watch for disconnections, so they expect the EG to be unwrapped

[graingert]

I mention that it's an optional background task in StreamingResponse, because it should not be the case that StreamingResponse raises a different kind of exception based on asgi version

[Kludex]

I don't want to change anything outside the base http middleware because of it

[graingert]

we can nest the raise app_exc inside the collapse_excgroups() in base.py - but it's not really correct

[graingert]

I'll have a think

[Kludex]

Do you have an MRE to share of a code that would be a problem with the raise app_exc inside the collapse_excgroups?

The PR you are working in seems to also change the StreamingResponse, so it's not really avoiding changes in it.

[graingert]

so the raise app_exc can't be put inside the async with of the task group because the app_exc might not be set until after the task group has finished, and you should not put anything in between a task group and collapse_excgroups because then you're collapsing exc groups that you've not created. Thus it has to be this way.

If you want to avoid touching StreamingResponse in this PR, I've separated the changes out into another PR that's more like the old anyio 3 behaviour and strict_exception_groups=False

[graingert]

There's a discussion ongoing in the trio gitter to make strict_exception_groups not deprecated and expose it in anyio, then I'd make a PR to use that where available

[graingert]

Do you have an MRE to share of a code that would be a problem with the raise app_exc inside the collapse_excgroups?

The PR you are working in seems to also change the StreamingResponse, so it's not really avoiding changes in it.

MRE converted to test cases and in #2830