Move BackgroundTask execution outside of middleware stack

[adriangb]

Fixes #1438, fixes #919, closes #1654, closes #1699

This will currently break backwards compatibility if someone tries to use the "new" Response implementation with an "old" Starlette app (or an app that is not Starlette, like FastAPI which copies a lot of Starlette code and will have to manually update 1 LOC). We could easily work around this with an if "starlette.background" in scope but it will require a couple more tests. Otherwise this should be backwards compatible.

[kigawas]

It's better to add some background task tests

https://github.com/encode/starlette/pull/1699/files#diff-f4ca7144ea672966ffaaabe2fc8e7aa2230b6b029a253e3b2dc4e1d9136d5b5fR89

[adriangb]

There is already a test which does not pass on master but passes on this branch. The test you linked also passes on master.

[kigawas]

tests/middleware/test_base.py::test_background_tasks[asyncio] background task 1 started
PASSED
tests/middleware/test_base.py::test_background_tasks[trio] background task 1 started
PASSED

It didn't actually "pass" the test on master - the background tasks are cancelled during the sleep(2).

The expected output is:

tests/middleware/test_base.py::test_background_tasks[asyncio] background task 1 started
background task 1 completed
background task 2 started
background task 2 completed
background task sync started
background task sync completed
PASSED
tests/middleware/test_base.py::test_background_tasks[trio] background task 1 started
background task 1 completed
background task 2 started
background task 2 completed
background task sync started
background task sync completed
PASSED

[adriangb]

Apologies, I did not look at the print statements. In this codebase print statements are not a valid way to make a test fail.

That said, you do make a point: I had added a test for #1438 but not #919. I added a test for #919 now.

[jhominal]

When I saw this PR about a week ago, I thought that this was a pretty good solution to #1438.

However, since then I have been able to think about that issue more deeply (after being prompted by another of your PRs, #1697), and I believe that #1715 also solves #1438, but keeps background tasks as being run in the request/response cycle.

I do not mean to say that "putting background tasks in a separate, app-initialized task group" is not a worthwhile change - I only mean to say that it may not be necessary to fix #1438.

[adriangb]

Glad one of my messy PRs actually inspired something instead of just being noise 😅

I do not mean to say that "putting background tasks in a separate, app-initialized task group" is not a worthwhile change

There is not TaksGroup in this PR. That's the name of the branch and that being the original idea, but I realized it was too ambitious so I toned it down (I still think a TaskGroup launched by a lifespan event would be the right thing to do, but not today).

I only mean to say that it may not be necessary to fix https://github.com/encode/starlette/issues/1438.

👍 gotcha I do think there's a lot to be said for not changing other parts of the codebase in an effort to fix bugs caused by BaseHTTPMiddleware. That said, I think this is a pretty natural change. I think that if I looked at this in 5 years without any comments it'd still make sense. Doesn't make it a better option per-se, but if this wasn't the case I think it'd be a bad option for sure.

[adriangb]

I ended up just removing the test for #919. I think if we end up merging one of these solutions we should probably comment on that issue saying "hey we think this is fixed and also this issue has become an agglomeration of possibly multiple bugs that have changed over time so we are closing it for now. If you are still suffering from this issue please open a new issue with a self contained example"

[gnat]

For reference I do believe this is the pattern employed by sanic: Task group outside of the middleware stack. https://sanic.dev/en/guide/basics/tasks.html

Seems sound.

[adriangb]

Also used by Quart: https://github.com/pgjones/quart-trio/blob/d0b775b2dda60cba916df6989c2e44fdf14f30d1/src/quart_trio/asgi.py#L114 https://github.com/pgjones/quart-trio/blob/d0b775b2dda60cba916df6989c2e44fdf14f30d1/src/quart_trio/app.py#L253

Quart uses a TaskGroup/Nuersery created in the lifespan event. That latter implementation was the original goal of this PR, I've thought that was the right move since before knowing Quart does that, which I think indicates it is indeed the right move long term. But for today this will have to do.