Prevent anyio.ExceptionGroup in error views under a BaseHTTPMiddleware#1262
Prevent anyio.ExceptionGroup in error views under a BaseHTTPMiddleware#1262florimondmanca merged 5 commits intomasterfrom
Conversation
ed4246f to
525215a
Compare
| assert response.headers["Custom-Header"] == "Example" | ||
|
|
||
| with pytest.raises(Exception): | ||
| with pytest.raises(Exception) as ctx: |
There was a problem hiding this comment.
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.
525215a to
58defbc
Compare
|
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" |
|
Hey team. Anything I can do to help push this along? I'm running into this issue as well, which is impacting 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? |
|
Looks like this is ready then — happy to have anyone review and release this. |
simondale00
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks, @florimondmanca!
|
Hello, we will be using this fix at our company too, looks good. |
|
Thanks! |
Closes #1255
Motivation
#1157 switched to using
anyiofor async concurrency operations.But now when a view raises an exception, say
MyExc, and aBaseHTTPMiddlewareis installed, calling the view would make Starlette raise ananyio.ExceptionGroup, rather thanMyExcdirectly.Previously, under
asyncio, we were callingtask.result()before raisingRuntimeError("No response sent."), so onlyMyExcwas raised, as expected, see here: https://github.com/encode/starlette/blob/f5a08d019ade528ae3a855f7e52a5500b233cb0a/starlette/middleware/base.py#L45-L46This PR switches back to that behavior by adding a
checkpoint(equivalent toasyncio.sleep(0)— i.e. "allow switching tasks and checking for cancellation) before raising the "No response sent" exception.cc @uSpike