Move BackgroundTask execution outside of middleware stack #1700

adriangb · 2022-06-22T17:36:26Z

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 · 2022-06-23T07:29:59Z

It's better to add some background task tests

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

adriangb · 2022-06-23T07:45:00Z

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 · 2022-06-23T07:55:28Z

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 · 2022-06-23T08:49:23Z

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 · 2022-07-02T06:17:43Z

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 · 2022-07-02T06:18:51Z

tests/test_background.py

+@pytest.fixture(
+    params=[[], [BackgroundTaskMiddleware]],
+    ids=["without BackgroundTaskMiddleware", "with BackgroundTaskMiddleware"],
+)
+def test_client_factory_mw(
+    test_client_factory: TestClientFactory, request: Any
+) -> TestClientFactory:
+    mw_stack: List[Callable[[ASGIApp], ASGIApp]] = request.param
+
+    def client_factory(app: ASGIApp) -> TestClient:
+        for mw in mw_stack:
+            app = mw(app)
+        return test_client_factory(app)
+
+    return client_factory
+
+
+def response_app_factory(task: BackgroundTask) -> ASGIApp:
+    async def app(scope: Scope, receive: Receive, send: Send):
+        response = Response(b"task initiated", media_type="text/plain", background=task)
+        await response(scope, receive, send)
+
+    return app
+
+
+def file_response_app_factory(task: BackgroundTask) -> ASGIApp:
+    async def app(scope: Scope, receive: Receive, send: Send):
+        with NamedTemporaryFile("wb+") as f:
+            f.write(b"task initiated")
+            f.seek(0)
+            response = FileResponse(f.name, media_type="text/plain", background=task)
+            await response(scope, receive, send)
+
+    return app
+
+
+def streaming_response_app_factory(task: BackgroundTask) -> ASGIApp:
+    async def app(scope: Scope, receive: Receive, send: Send):
+        async def stream() -> AsyncIterable[bytes]:
+            yield b"task initiated"
+
+        response = StreamingResponse(stream(), media_type="text/plain", background=task)
+        await response(scope, receive, send)
+
+    return app


This is all just to get 100% code coverage with both code paths. If we removed the old code path (i.e. backgroundTask does not work without the middleware) there would be (almost) no changes to these tests and it would look a lot like https://github.com/xpresso-devs/asgi-background

adriangb · 2022-07-02T06:26:08Z

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 #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.

jhominal

I have realized that I did not actually read your test case for #1438 in detail, so I made a few comments.

I do realize that these comments would make the tests for #1438 and #919 very close, but as I believe the underlying issue is the same, that is not wholly unexpected.

tests/middleware/test_base.py

Co-authored-by: Jean Hominal <jhominal@gmail.com>

adriangb · 2022-07-02T15:56:07Z

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 · 2022-07-08T02:42:28Z

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 · 2022-07-08T02:48:22Z

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.

adriangb added 5 commits June 22, 2022 10:34

Move BackgroundTask execution outside of middleware stack

5b15af4

add middleware src file

19878ed

remove extra list

c313791

remove sleeps

72e14aa

remove aclosing

cfc83a2

adriangb requested a review from Kludex June 22, 2022 17:45

adriangb added 2 commits June 23, 2022 01:25

add test

bcec47d

add test

c9ac227

jhominal mentioned this pull request Jul 1, 2022

Replace task cancellation in BaseHTTPMiddleware with http.disconnect+recv_stream.close #1715

Merged

2 tasks

adriangb added 4 commits July 2, 2022 00:23

Merge branch 'master' into task-group

f5cb1ee

start to make backwards compatible

8b43cec

make backwards compatible

4bade40

remove whitespace

d6659a5

adriangb commented Jul 2, 2022

View reviewed changes

minimize diff

c6fcfac

jhominal reviewed Jul 2, 2022

View reviewed changes

tests/middleware/test_base.py Outdated Show resolved Hide resolved

tests/middleware/test_base.py Outdated Show resolved Hide resolved

adriangb and others added 6 commits July 2, 2022 09:47

Update tests/middleware/test_base.py

7d8b50b

Co-authored-by: Jean Hominal <jhominal@gmail.com>

Update tests/middleware/test_base.py

6cb1b06

Co-authored-by: Jean Hominal <jhominal@gmail.com>

fix test

5e6b2bc

improve test

b9a54c8

remove test

c0af53d

remove import

072e949

Merge branch 'master' into task-group

d8b7fce

adriangb marked this pull request as ready for review July 8, 2022 02:49

Kludex added this to the Version 0.21.0 milestone Jul 22, 2022

Kludex added the refactor Refactor code label Jul 22, 2022

Kludex modified the milestones: Version 0.21.0, Version 1.x Sep 3, 2022

Kludex closed this in #1715 Sep 24, 2022

adriangb mentioned this pull request Jun 5, 2023

HTTPMiddleware breaks BackgroundTasks in the same connection #2093

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move BackgroundTask execution outside of middleware stack #1700

Move BackgroundTask execution outside of middleware stack #1700

adriangb commented Jun 22, 2022 •

edited

Loading

kigawas commented Jun 23, 2022

adriangb commented Jun 23, 2022

kigawas commented Jun 23, 2022

adriangb commented Jun 23, 2022

jhominal commented Jul 2, 2022 •

edited

Loading

adriangb Jul 2, 2022 •

edited

Loading

adriangb commented Jul 2, 2022

jhominal left a comment

adriangb commented Jul 2, 2022

gnat commented Jul 8, 2022

adriangb commented Jul 8, 2022 •

edited

Loading

Move BackgroundTask execution outside of middleware stack #1700

Move BackgroundTask execution outside of middleware stack #1700

Conversation

adriangb commented Jun 22, 2022 • edited Loading

kigawas commented Jun 23, 2022

adriangb commented Jun 23, 2022

kigawas commented Jun 23, 2022

adriangb commented Jun 23, 2022

jhominal commented Jul 2, 2022 • edited Loading

adriangb Jul 2, 2022 • edited Loading

Choose a reason for hiding this comment

adriangb commented Jul 2, 2022

jhominal left a comment

Choose a reason for hiding this comment

adriangb commented Jul 2, 2022

gnat commented Jul 8, 2022

adriangb commented Jul 8, 2022 • edited Loading

adriangb commented Jun 22, 2022 •

edited

Loading

jhominal commented Jul 2, 2022 •

edited

Loading

adriangb Jul 2, 2022 •

edited

Loading

adriangb commented Jul 8, 2022 •

edited

Loading