Background tasks are cancelled if the client closes connection

[jhominal]

Checklist

• [X] The bug is reproducible against the latest release or master.
• [X] There are no similar issues or pull requests to fix it yet.

Describe the bug

When the HTTP client closes the TCP socket immediately after receiving the HTTP response, background tasks are cancelled.

This bug only happens when running the ASGI under uvicorn, and only if at least one HTTP Middleware is defined in the user middleware chain.

Steps to reproduce the bug

1. Write the following ASGI Starlette application in repro.py:

import traceback

import anyio
from starlette.applications import Starlette
from starlette.background import BackgroundTasks
from starlette.middleware import Middleware
from starlette.middleware.base import BaseHTTPMiddleware
from starlette.responses import Response
from starlette.routing import Route


async def passthrough(request, call_next):
    return await call_next(request)


async def _sleep(identifier, delay):
    print(identifier, "started")
    try:
        await anyio.sleep(delay)
        print(identifier, "completed")
    except BaseException:
        print(identifier, "error")
        traceback.print_exc()
        raise


async def response_with_sleeps(request):
    background_tasks = BackgroundTasks()
    background_tasks.add_task(_sleep, "background task 1", 2)
    background_tasks.add_task(_sleep, "background task 2", 2)
    return Response(background=background_tasks)


application = Starlette(
    middleware=[
        Middleware(BaseHTTPMiddleware, dispatch=passthrough),
    ],
    routes=[
        Route("/", response_with_sleeps),
    ],
)

2. Run that application using uvicorn (either uvloop or regular asyncio will reproduce the issue) on localhost:8000

uvicorn repro:application --port 8000

3. Run the following client script

#!/usr/bin/env python
import socket

connection = socket.create_connection(("localhost", 8000))
connection.sendall(b"GET / HTTP/1.1\r\nHost: localhost\r\n\r\n")
print(connection.recv(10000).decode("utf8"))
connection.close()

Expected behavior

The client script gets the HTTP response, and both background tasks should complete successfully.

The expected behavior will be detectable by the following content in standard output:

background task 1 started
background task 1 completed
background task 2 started
background task 2 completed

Actual behavior

Background task 1 is interrupted at the await point and background task 2 is never started.

That results in the following content in the output (when running the repro.py application):

background task 1 started
background task 1 error
Traceback (most recent call last):
  File "/Users/jean/PycharmProjects/starlette-bg-cancelled/./repro.py", line 19, in _sleep
    await anyio.sleep(delay)
  File "/Users/jean/PycharmProjects/starlette-bg-cancelled/venv/lib/python3.9/site-packages/anyio/_core/_eventloop.py", line 69, in sleep
    return await get_asynclib().sleep(delay)
  File "/usr/local/Cellar/[email protected]/3.9.6/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/tasks.py", line 654, in sleep
    return await future
asyncio.exceptions.CancelledError

Debugging material

No response

Environment

• MacOS 10.14.6 / Python 3.9 / Starlette 0.18.0

Additional context

• When I remove the passthrough middleware, the bug goes away.
• When I run the same application in hypercorn, the bug goes away.
• There does not seem to be a difference between using uvloop or not.
• If the client script (e.g. with a time.sleep(10)) maintains the TCP connection open, the bug goes away.

[Kludex]

Thanks for the precise report @jhominal :)

I've implemented a solution on #1441. Let's see how the discussion around that goes.

[xingdongzhe]

I looked the code that BaseHTTPMiddleware and StreamingResponse use the same task_group, when one scope receive http.disconnect, task_group cancel all taskes. But I don't know why it can cancel Response.backgroundtasks. Do I missing other thing?

[Kludex]

The PR that solves the issue has all the details.

EDIT: My PR had some regressions. I've closed it.

[kigawas]

The background tasks need to be shielded.

diff --git a/starlette/background.py b/starlette/background.py
index 4aaf7ae..db9b38a 100644
--- a/starlette/background.py
+++ b/starlette/background.py
@@ -1,6 +1,8 @@
 import sys
 import typing
 
+import anyio
+
 if sys.version_info >= (3, 10):  # pragma: no cover
     from typing import ParamSpec
 else:  # pragma: no cover
@@ -22,10 +24,11 @@ class BackgroundTask:
         self.is_async = is_async_callable(func)
 
     async def __call__(self) -> None:
-        if self.is_async:
-            await self.func(*self.args, **self.kwargs)
-        else:
-            await run_in_threadpool(self.func, *self.args, **self.kwargs)
+        with anyio.CancelScope(shield=True):
+            if self.is_async:
+                await self.func(*self.args, **self.kwargs)
+            else:
+                await run_in_threadpool(self.func, *self.args, **self.kwargs)