Move BackgroundTask execution outside of request/response cycle #2176
Conversation
e1a7d4f to
c19dd7c
Compare
|
@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? |
|
Since #2093 was solved, can we close this? |
|
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. |
|
I don't think this should get in before 1.0. We should probably discuss it first. |
|
Why not target 1.0 so that any observable behavior changes are not problematic? But sure let’s discuss it. |
|
Also, changing implementation for the sake of preventing future unknown bugs (that can never exist), can actually introduce bugs. |
|
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. |
|
@adriangb Do you still want this? |
|
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 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. |
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 |
|
I think we’ll come to regret it if we go 1.0 |
|
We can always release 2.0, if that happens. |
|
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. |
|
Thanks for pinging me! And thanks for the patience with my response. About FastAPIFrom 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 hadI 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 MiddlewareAbout 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 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 To 1.0 or not 1.0, that is the questionAbout 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. ErrorsI wonder what would hap
8000
pen if there's an error/exception in a task. I think it would bubble up to the |
|
As you point out, making this change now allows for future changes. For example, we could add a
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. |
|
@Kludex has the feedback here changed your mind at all? |
|
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.) |
|
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. |
|
I’d be okay with that |
|
Hi folks, new bugs with BaseHTTPMiddleware keep popping up... this seems to solve #2640 |
| [Middleware(ServerErrorMiddleware, handler=error_handler, debug=debug)] | ||
| + [Middleware(BackgroundTaskMiddleware)] |
There was a problem hiding this comment.
| [Middleware(ServerErrorMiddleware, handler=error_handler, debug=debug)] | |
| + [Middleware(BackgroundTaskMiddleware)] | |
| [ | |
| Middleware(ServerErrorMiddleware, handler=error_handler, debug=debug), | |
| Middleware(BackgroundTaskMiddleware), | |
| ] |
| finally: | ||
| 6D40 for task in tasks: | ||
| await task() |
There was a problem hiding this comment.
Background tasks should only run on 2xx.
I guess you'll need a wrap_send to get the status code.
| and background.is_background_task_middleware_installed(scope) | ||
| ): | ||
| background.add_tasks(scope, self.background) | ||
| self.background = None |
There was a problem hiding this comment.
What's the scenario you are trying to prevent when setting background to None? What if a user has a global response object?
response = Response(background=BackgroundTasks(...))There was a problem hiding this comment.
Ah, I see now. You are doing because you didn't remove the logic below where we run the background task.
I think we can remove that logic, and you can avoid this line.
(Is my understanding correct?)
| if _SCOPE_KEY not in scope: # pragma: no cover | ||
| raise RuntimeError( | ||
| "`add_tasks` can only be used if `BackgroundTaskMIddleware is installed" | ||
| ) |
There was a problem hiding this comment.
Can you trigger this, tho? If we always call is_background_task_middleware_installed before the add_tasks, then this will never be called. I guess the point you are trying to make here is to make this as public API, but enforce it for others.
We have some scenarios:
- You are running
Starlette(), which you always have theBackgroundTasksMiddleware- in this case we wouldn't need to check the scopes, because we know thescope[_SCOPE_KEY]is there. - You are running only parts of Starlette e.g.
Response- in this case, you don't have theBackgroundTasksMiddleware, which means this would be a breaking change - oh... Now I see what you did. You kept the previous behavior where it runs the background task if standaloneResponsewith background tasks is used.
Ok... As you see, I was figuring stuff as I was writing this to myself.
There was a problem hiding this comment.
I don't like that now the logic is different on those scenarios. I'm having a hard time figuring out how to avoid it tho...
We are in a similar situation with the exception of middleware.
There was a problem hiding this comment.
Actually... If the _SCOPE_KEY is not present, we can error as above, which will make it clear to users that they need to wrap their responses with BackgroundTasksMiddleware.
What I mean is to not check if the BackgroundTasksMiddleware is installed, and error if user tries to use it outside the Starlette application.
Maybe that's your plan but with a deprecation warning?
|
I'm not convinced this is the way to go. It seems the only issues with the background tasks are related to |
I feel like there was something else broken that this fixed but given that it's been years now it's hard to remember. I'll still make the point that aside from fixing the bugs with BaseHTTPMiddleware this also:
|
|
I agree that moving the task out of |
Fixes #2640