Memory usage streaming large responses

[via]

We've been running into memory issues when providing very large async generators to a streaming response. We have these generators producing large (larger than memory set) responses in a way that allows us to only keep small chunks in memory at a time. However, it looks like the BaseHTTPMiddleware implementation uses an asyncio queue to store the individual chunks:

https://github.com/encode/starlette/blob/master/starlette/middleware/base.py#L30

This prevents any network backpressure handling -- if the client that is receiving the streaming response is on a slow connection, the queue will happily grow without bound and consume all memory, triggering kernel out-of-memory, when the ideal handling here would be for send to block (yield) when this happens. I believe this would naturally happen if there were no queue here at all, so I am wondering why it needs to be here?

Would a PR to remove the queueing be accepted?

If not, what is the appropriate way to override this to not use a queue? We can write our own, but the use of BaseHTTPMiddleware is hardcoded:

starlette/starlette/applications.py

Line 197 in 519f575

self.add_middleware(BaseHTTPMiddleware, dispatch=func)

, leaving only some fairly hacky approaches to preventing this queueing.

[euri10]

I kind of remember it has been discussed already here, not sure it's relevant to your case though #802

…

On Mon, Jul 27, 2020, 7:23 PM via ***@***.***> wrote: We've been running into memory issues when providing very large async generators to a streaming response. We have these generators producing large (larger than memory set) responses in a way that allows us to only keep small chunks in memory at a time. However, it looks like the BaseHTTPMiddleware implementation uses an asyncio queue to store the individual chunks: https://github.com/encode/starlette/blob/master/starlette/middleware/base.py#L30 This prevents any network backpressure handling -- if the client that is receiving the streaming response is on a slow connection, the queue will happily grow without bound and consume all memory, triggering kernel out-of-memory, when the ideal handling here would be for send to block (yield) when this happens. I believe this would naturally happen if there were no queue here at all, so I am wondering why it needs to be here? Would a PR to remove the queueing be accepted? If not, what is the appropriate way to override this to not use a queue? We can write our own, but the use of BaseHTTPMiddleware is hardcoded: https://github.com/encode/starlette/blob/519f5750b5e797bb3d4805fd29657674304ce397/starlette/applications.py#L197, leaving only some fairly hacky approaches to preventing this queueing. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1012>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINSPT7UOTUWBXAWKM352TR5WZX5ANCNFSM4PJAVLSA> .

[via]

I did see that ticket, but I do think its important to distinguish between the high level backpressure logic that I think that ticket is mostly about (specifically managing concurrent request limits, etc), and network backpressure. I do not believe that ticket is correct when it suggests that uvicorn/gnuicorn manages the low-level network socket backpressure, as the use of the asyncio.Queue there is completely in starlette, and will happily absorb the entire response payload until it dies.

[florimondmanca]

I think queueing is fine, but the queue is unbounded which results in losing the backpressure properties provided by the server. (As you said, it’s allowed to grow forever even if the other side isn’t consuming chunks fast enough.)

In my mind setting a maxsize of eg 10 would resolve the memory issues to you’re seeing, because then if the client isn’t consuming chunks fast enough the app will block on send() until there’s room in the queue.

Edit: actually as discussed by @erewok in the chat, a maxsize of 1 should be just enough. This way we stick to the backpressure behavior of the server as close as we can, and I don't think there's a risk of deadlock. In any case, any fix would be worth validating on a real-life big upload too. :-)

[erewok]

For a bit of background, the BaseHTTPMiddleware offers a way for users to write middleware that takes a request and builds a response. This is a bit closer to the request-handling logic than other middleware, but in order to assemble a response, the BaseHTTPMiddleware needs some generic way to handle any potential response the handler would produce, which is the reason for the queue and streaming.

Also, as @florimondmanca mentioned above, @via If you have an easy opportunity to do so, you may consider experimentally running the branch from PR #1017 (branch: stream_response_bg) and seeing if your memory issues in the case of large responses are resolved. If you do find an opportunity to try it, please report back.

[erewok]

@via Starlette version 0.13.7 has been released to address this issue.

[via]

@erewok thank you for the information!

We have been using a similar change (queue size of 3), and were going to try the PR you referenced. We will just update and can report back. Thank you for the speedy responses.

[erewok]

Due to issues with unevaluated responses, we have elected to revert the maxsize fix (see PR #1028 ), and so I want to leave this issue open for now hopefully as guidance to anyone who encounters the same issue. A documentation update will be provided with this information soon as well.

As stated in the related issue on streaming with BaseHTTPMiddleware (#919):

• We would like to discourage the use of BaseHTTPMiddleware in favor of raw ASGI middleware which includes a method def __call__(self, scope, receive, send):, and

• If people still want to use this middleware class, it should never be used with StreamingResponse/FileResponse endpoints.

Unfortunately, while the BaseHTTPMiddleware class offers an easier interface because it provides a request argument and promises something like a response, this middleware class also encourages users to think about the asgi app functionality in a "complete" or "finished" way. This means this class will either load the entirety of streaming requests into memory (this issue) and run the background before returning the response (#919 ), or if we fix those problems, that it will then encourage users to leave resources in a pending or open state, an arguably worse result. In short, it's problematic.

Again, these problems should be absent if you avoid subclassing BaseHTTPMiddleware.

[JayH5]

I believe this should be fixed by #1157. We now use an AnyIO stream in BaseHTTPMiddleware that is not unbounded:

starlette/starlette/middleware/base.py

Line 26 in 7ed2890

send_stream, recv_stream = anyio.create_memory_object_stream()

We've also avoided the problem described in #1022 (context).