Conversation
|
FYI, The streaming response used to also do: It itsn't covered so when it was removed in https://github.com/encode/starlette/pull/2620/files it didn't break |
| 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? |
Alternative to #2176