Skip to content

Shield background tasks #1654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Shield background tasks #1654

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1b46d3
Shield background tasks
Kludex May 31, 2022
0b4ff99
Update starlette/background.py
Kludex May 31, 2022
57f5d73
Update starlette/background.py
Kludex May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions starlette/background.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import sys
import typing
import anyio

if sys.version_info >= (3, 10): # pragma: no cover
from typing import ParamSpec
Expand Down Expand Up @@ -39,5 +40,7 @@ def add_task(
self.tasks.append(task)

async def __call__(self) -> None:
for task in self.tasks:
await task()
async with anyio.create_task_group():
with anyio.CancelScope(shield=True):
Copy link
Contributor

@adriangb adriangb Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this task group needed? I think just the cancel scope should be enough.

for task in self.tasks:
Comment on lines +45 to +46
Copy link
Contributor

@adriangb adriangb Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this opt in on a per-task basis? For backwards compatibility and because I don't think in some cases it does make sense to cancel the background task if the client closes the connection.

Copy link
Owner Author

@Kludex Kludex Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which cases?

Copy link
Contributor

@adriangb adriangb Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Business logic. Like if you're doing something that is expensive and you don't care if it succeeds or fails if the client closes the connection. No specific use case in mind, just thinking in general.

Although now I realize this is even more complicated because the behavior changes depending on if there is a BaseHTTPMiddleware involved, right? What happens if there isn't a BaseHTTPMiddleaware involved? Does it depend on the ASGI server?

Copy link
Owner Author

@Kludex Kludex Jun 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fails if the client closes the connection

bkg tasks runs after the response 🤔

Copy link
Contributor

@adriangb adriangb Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was maybe getting a bit confused as to what actually causes the issue.

await task()