Repository: encode/starlette

Issue:
Background tasks exception handler
Currently, if a background task faces an exception, it isn't handled and there's no way to handle it (rather than wrapping the task)
My suggestion is to be able to add exception handler for background tasks, as there is for requests.

Discussion:
aviramha: Should I add a test that checks in case a response started that the handler is still called?
aviramha: @tomchristie Thoughts?
florimondmanca: @aviramha I'm not sure I understand the use case here. Can you clarify why we'd need to move the call to the installed error handler separately from the other cases?
aviramha: > @aviramha I'm not sure I understand the use case here. Can you clarify why we'd want/need to move the call to the installed error handler separately from the other cases?

The use case are exception handlers not being called on background tasks since the response already started. This changes the logic so the handlers will be called even if response is returned. It's extremely important for usage of error loggers like sentry etc. 
aviramha: Any update? :)
Kludex: I'll wait to see if someone else wants to comment here.
adriangb: I think this makes sense, for example if users log within their exception handlers.

My main two concerns would be:

- How this interacts with the work to get exception handlers for websockets
- We'll have to document the behavior. I could see users getting confused as to why the response they return from error handlers isn't used
Kludex: > How this interacts with the work to get exception handlers for websockets

This is only for HTTP, no interaction.


Kludex: Docs added. Suggestions welcome about the wording, if any.
aviramha: Looks great. Thanks @Kludex for seeing this through.
Kludex: @tomchristie Would you like to get the last :+1: here?

Patch:
diff --git a/docs/exceptions.md b/docs/exceptions.md
index bf460d2..9818a20 100644
--- a/docs/exceptions.md
+++ b/docs/exceptions.md
@@ -75,6 +75,24 @@ should bubble through the entire middleware stack as exceptions. Any error
 logging middleware should ensure that it re-raises the exception all the
 way up to the server.
 
+In practical terms, the error handled used is `exception_handler[500]` or `exception_handler[Exception]`.
+Both keys `500` and `Exception` can be used. See below:
+
+```python
+async def handle_error(request: Request, exc: HTTPException):
+    # Perform some logic
+    return JSONResponse({"detail": exc.detail}, status_code=exc.status_code)
+
+exception_handlers = {
+    Exception: handle_error  # or "500: handle_error"
+}
+```
+
+It's important to notice that in case a [`BackgroundTask`](https://www.starlette.io/background/) raises an exception,
+it will be handled by the `handle_error` function, but at that point, the response was already sent. In other words,
+the response created by `handle_error` will be discarded. In case the error happens before the response was sent, then
+it will use the response object - in the above example, the returned `JSONResponse`.
+
 In order to deal with this behaviour correctly, the middleware stack of a
 `Starlette` application is configured like this:
 
diff --git a/starlette/middleware/errors.py b/starlette/middleware/errors.py
index 30f5570..474c9af 100644
--- a/starlette/middleware/errors.py
+++ b/starlette/middleware/errors.py
@@ -158,21 +158,21 @@ class ServerErrorMiddleware:
         try:
             await self.app(scope, receive, _send)
         except Exception as exc:
-            if not response_started:
-                request = Request(scope)
-                if self.debug:
-                    # In debug mode, return traceback responses.
-                    response = self.debug_response(request, exc)
-                elif self.handler is None:
-                    # Use our default 500 error handler.
-                    response = self.error_response(request, exc)
+            request = Request(scope)
+            if self.debug:
+                # In debug mode, return traceback responses.
+                response = self.debug_response(request, exc)
+            elif self.handler is None:
+                # Use our default 500 error handler.
+                response = self.error_response(request, exc)
+            else:
+                # Use an installed 500 error handler.
+                if asyncio.iscoroutinefunction(self.handler):
+                    response = await self.handler(request, exc)
                 else:
-                    # Use an installed 500 error handler.
-                    if asyncio.iscoroutinefunction(self.handler):
-                        response = await self.handler(request, exc)
-                    else:
-                        response = await run_in_threadpool(self.handler, request, exc)
+                    response = await run_in_threadpool(self.handler, request, exc)
 
+            if not response_started:
                 await response(scope, receive, send)
 
             # We always continue to raise the exception.


Test patch:
diff --git a/tests/middleware/test_errors.py b/tests/middleware/test_errors.py
index 2c926a9..392c2ba 100644
--- a/tests/middleware/test_errors.py
+++ b/tests/middleware/test_errors.py
@@ -1,7 +1,10 @@
 import pytest
 
+from starlette.applications import Starlette
+from starlette.background import BackgroundTask
 from starlette.middleware.errors import ServerErrorMiddleware
 from starlette.responses import JSONResponse, Response
+from starlette.routing import Route
 
 
 def test_handler(test_client_factory):
@@ -68,3 +71,28 @@ def test_debug_not_http(test_client_factory):
         client = test_client_factory(app)
         with client.websocket_connect("/"):
             pass  # pragma: nocover
+
+
+def test_background_task(test_client_factory):
+    accessed_error_handler = False
+
+    def error_handler(request, exc):
+        nonlocal accessed_error_handler
+        accessed_error_handler = True
+
+    def raise_exception():
+        raise Exception("Something went wrong")
+
+    async def endpoint(request):
+        task = BackgroundTask(raise_exception)
+        return Response(status_code=204, background=task)
+
+    app = Starlette(
+        routes=[Route("/", endpoint=endpoint)],
+        exception_handlers={Exception: error_handler},
+    )
+
+    client = test_client_factory(app, raise_server_exceptions=False)
+    response = client.get("/")
+    assert response.status_code == 204
+    assert accessed_error_handler