Move BackgroundTask execution outside of request/response cycle

[adriangb]

Fixes #2093

[adriangb]

@tiangolo you mentioned in the FastAPI PR that you’d endorse this change and the consequences for FastAPI. If so, would you like to review here / state that on this PR?

[Kludex]

Since #2093 was solved, can we close this?

[adriangb]

No I think this is still worth considering for 1.0. It has value outside of solving that one bug. It’s a more sensible execution model, and I believe it will prevent related bugs in the future, eg if someone uses tasks in their own ASGI middleware.

[Kludex]

I don't think this should get in before 1.0. We should probably discuss it first.

[adriangb]

Why not target 1.0 so that any observable behavior changes are not problematic?

But sure let’s discuss it.

[Kludex]

Also, changing implementation for the sake of preventing future unknown bugs (that can never exist), can actually introduce bugs.

[adriangb]

It's not just future bugs in Starlette, it's simplifying the execution model to avoid bugs in users' code, making it easier to time requests, adding a feature in the future to make background tasks a protocol so you can seamlessly switch from local execution like Starlette currently does to putting it on a queue, etc.

[Kludex]

@adriangb Do you still want this?

[adriangb]

I still think this is a good idea. I spoke with @tiangolo and he thought so as well, despite any changes required in FastAPI. However no one has shown a strong opinion in favor or against. What do you think we should do?

[UberKitten]

I support this change, I think it better matches up with what users would expect from a feature named "BackgroundTasks". I imagine the current implementation is already creating a lot of subtle bugs that simply don't get noticed most of the time.

Reading the docs for BackgroundTasks and using them a handful of times in production, I honestly thought this behavior was how it already worked. This was mostly an assumption based on the name BackgroundTasks and this statement in the docs: "A background task should be attached to a response, and will run only once the response has been sent."

Recently I ran into some issues related to #2093 and I started digging in to the code. Only then did I discover that this was not the case. An alternate change could be to clarify the execution model in the docs and make it more obvious how they currently work.

[Kludex]

I still think this is a good idea. I spoke with @tiangolo and he thought so as well, despite any changes required in FastAPI. However no one has shown a strong opinion in favor or against. What do you think we should do?

I also think this is a better design, but I'd like to avoid changes that are not a clear win. Even if the current design brought issues in the past, we are not aware of current issues, and trying to predict them doesn't make sense.

I think we should close the PR for the time being, and if some issue related to BackgroundTasks appears again, then we can reconsider this.

[adriangb]

I think we’ll come to regret it if we go 1.0

[Kludex]

We can always release 2.0, if that happens.

[wu-clan]

Since this is a good proposal, why not in 1.0?

1.0 has been implemented a lot, and even if there are problems here, fixes can be released instead of 2.0 at any time.

[tiangolo]

Thanks for pinging me! And thanks for the patience with my response.

About FastAPI

From the point of view of FastAPI, now that in recent versions dependencies are finished before background tasks, this would not affect FastAPI (it would have affected it before, but not anymore 😎).

So, taking this PR or not would be fine for FastAPI (I think).

Ideas I had

I had some ideas of allowing some execution of code (background tasks-ish) independent from the request/response cycle.

But the ideas I had were even a bit more drastic than this, putting tasks in one single big list/queue for the whole app, and each request would add to that giant list/queue, and it would be executed in its own time, independent of the request/response lifecycle.

BackgroundTasks in Middleware

About adopting this specific PR or not, I'm personally undecided (just my opinion).

I agree it's a simpler system, and it would allow doing something like having a potential future custom middleware that would put the tasks in some external service or something, so that's a potential plus for the future.

The other thing I see is that this gets closer to my idea of having code execution independent of the requests, but in this case it's independent of the Request object, not of the request/response lifecycle, because it still depends on the middlewares, so, a list of background tasks per request, executed after the request... so I see that it's not as far away from what is currently happening. 🤔

The other interesting thing is that this PR, has a sort of implicit opt-out mechanism, if people want to use the old behavior, they could remove the middleware, and then the regular execution as part of the Request object would happen, just like a quick escape hatch in case someone found a problem with this and needed a quick way to solve it.

To 1.0 or not 1.0, that is the question

About taking this before or after 1.0, if it's agreed by others that this is a better implementation, I personally would prefer to take it before 1.0, I would try to break as many known changes as needed before 1.0, to be able to revert them when necessary before 1.0, instead of releasing 1.0, then 2.0 with this, and then right after 3.0 reverting this because there was an unexpected issue with it. That's the same reason why I'm taking my time to release FastAPI 1.0, I want to make all the needed breaking changes and reverts while in 0.x land.

Errors

I wonder what would happen if there's an error/exception in a task. I think it would bubble up to the ServerErrorMiddleware, or not? Because then it would try to handle it and return a 500, but that was already done, so the ServerErrorMiddleware itself would error out. 🤔

[adriangb]

As you point out, making this change now allows for future changes. For example, we could add a run_in_taskgroup=True parameter or some sort of runner: ImplementsSomeRunnerProtocol to run on a job queue. Any similarity to the current implementation is just to maximize backward compatibility and minimize unintended changes for users.

I wonder what would happen if there's an error/exception in a task. I think it would bubble up to the ServerErrorMiddleware, or not? Because then it would try to handle it and return a 500, but that was already done, so the ServerErrorMiddleware itself would error out.

That's a good question. I feel like any errors in background tasks should be logged but otherwise ignored. Or we can put this middleware outside of the ServerErrorMiddleware and let them bubble up to the ASGI server. Then there's questions like if one task fails should all of them fail or should the rest execute, etc. But that seems like relatively minor issues and I'm happy to resolve them in whatever direction reviewers prefer. Probably best to keep the behavior the same as the current behavior for now.

[adriangb]

@Kludex has the feedback here changed your mind at all?

[Kludex]

Any change is susceptible to unpredictable bugs, and we don't have any issues with the current implementation at the moment. I don't think we should make this change.

(Also, the reason for this PR was to fix a bug that was fixed on a different PR.)

[alex-oleshkevich]

Unpopular opinion, but what if we drop background tasks from the Starlette itself? Let that feature be in 3rd-party. Maintaining tasks in web framework feels wrong to me and in-house reimplementation is not so hard.

[adriangb]

I’d be okay with that