Prevent anyio.ExceptionGroup in error views under a BaseHTTPMiddleware

[florimondmanca]

Closes #1255

Motivation
#1157 switched to using anyio for async concurrency operations.

But now when a view raises an exception, say MyExc, and a BaseHTTPMiddleware is installed, calling the view would make Starlette raise an anyio.ExceptionGroup, rather than MyExc directly.

Previously, under asyncio, we were calling task.result() before raising RuntimeError("No response sent."), so only MyExc was raised, as expected, see here:

starlette/starlette/middleware/base.py

Lines 45 to 46 in f5a08d0

	task.result()
	raise RuntimeError("No response returned.")

This PR switches back to that behavior by adding a checkpoint (equivalent to asyncio.sleep(0) — i.e. "allow switching tasks and checking for cancellation) before raising the "No response sent" exception.

cc @uSpike

[florimondmanca]

When running on trio, previously we would be getting an unexpected "Portal is not running" error here, and misinterpreting it as the Exception() in the /exc view. So I made the latter more precise and made sure we're explicitly checking for it.

[uSpike]

I'm not sure I'm in favor of relying on event loop magic to correctly propagate exceptions.

Here's an idea that I think is a bit easier to follow, thoughts?:

diff --git a/starlette/middleware/base.py b/starlette/middleware/base.py
index bf337b8..30ae1ed 100644
--- a/starlette/middleware/base.py
+++ b/starlette/middleware/base.py
@@ -24,21 +24,25 @@ class BaseHTTPMiddleware:
 
         async def call_next(request: Request) -> Response:
             send_stream, recv_stream = anyio.create_memory_object_stream()
+            app_exc: typing.Optional[Exception] = None
 
             async def coro() -> None:
                 async with send_stream:
-                    await self.app(scope, request.receive, send_stream.send)
+                    try:
+                        await self.app(scope, request.receive, send_stream.send)
+                    except Exception as exc:
+                        nonlocal app_exc
+                        app_exc = exc
 
             task_group.start_soon(coro)
 
             try:
                 message = await recv_stream.receive()
             except anyio.EndOfStream:
-                # HACK: give anyio a chance to surface any app exception first,
-                # in order to avoid an `anyio.ExceptionGroup`.
-                # See #1255.
-                await anyio.lowlevel.checkpoint()
-                raise RuntimeError("No response returned.")
+                if app_exc is not None:
+                    raise app_exc
+                else:
+                    raise RuntimeError("No response returned.")
 
             assert message["type"] == "http.response.start"

[simondale00]

Hey team. Anything I can do to help push this along?

I'm running into this issue as well, which is impacting Exception handling. Specifically, adding any user middleware breaks ServerErrorMiddleware, as ServerErrorMiddleware is not built to catch ExceptionGroup. (While one could argue ServerErrorMiddleware could be modified to handle exception groups, I think the core issue here is that BaseHTTPMiddleware dismisses the possibility that a callable raises an exception and always expects to read a response)

I've tested out the proposal from @uSpike and it works well. Also seems like a pragmatic approach to resolve the issue.

When can we get this merged and released?

[florimondmanca]

Looks like this is ready then — happy to have anyone review and release this.

[simondale00]

Looks good to me. Thanks, @florimondmanca!

[kissgyorgy]

Hello, we will be using this fix at our company too, looks good.

[florimondmanca]

@Kludex Hi, saw you open #1324. I'd need a ✅ around here to merge. Thanks!

[Kludex]

This solution looks much simpler 👍

I was waiting to see if @uSpike wanted to say something, but the code looks pretty straightforward. 😄

[florimondmanca]

Thanks!