Fix BackgroundTasks with BaseHTTPMiddleware #2688
Conversation
|
FYI, The streaming response used to also do: It itsn't covered so when it was removed in https:/encode/starlette/pull/2620/files it didn't break |
| @@ -222,3 +222,6 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |||
| await send({"type": "http.response.body", "body": chunk, "more_body": True}) | |||
|
|
|||
| await send({"type": "http.response.body", "body": b"", "more_body": False}) | |||
|
|
|||
| if self.background: | |||
| await self.background() | |||
There was a problem hiding this comment.
For full backward compatibility add it to constructor also:
class _StreamingResponse(Response):
def __init__(
self,
content: AsyncContentStream,
status_code: int = 200,
headers: typing.Mapping[str, str] | None = None,
media_type: str | None = None,
background: BackgroundTask | None = None, <<<
info: typing.Mapping[str, typing.Any] | None = None,
) -> None:
self.info = info
self.body_iterator = content
self.status_code = status_code
self.media_type = media_type
self.background = background <<<
self.init_headers(headers)
There was a problem hiding this comment.
This is because some middleware may be referring to that field. Or below will work also
self.background = None
There was a problem hiding this comment.
No users should be calling the constructor unless they do something like type(response)(...) which is not something we need to support.
There was a problem hiding this comment.
Agree, this aligns with the class being marked as private. Then please include
self.background = None
For full backward compatibility, any response object injected into BaseHTTPMiddleware based middleware needs to have the background field (default None)
There was a problem hiding this comment.
Ok I see your point now with the edit. Good point. Added.
@Kludex this is another reason to move the background task logic out of responses and a good reason to rework our Response inheritance so that Response and StreamingResponse both inherit from a BaseResponse that has the API and initialization of common bits.
|
There is a test already, am I missing something? |
* Streaming response early disconnect mode * Fix BackgroundTasks with BaseHTTPMiddleware * move comment * initialize field --------- Co-authored-by: Dmitry Maliuga <[email protected]>
Alternative to #2176