http icon indicating copy to clipboard operation
http copied to clipboard

RequestBodyBufferMiddleware should limit total memory consumption for concurrent requests

Open clue opened this issue 8 years ago • 4 comments

The RequestBodyParserMiddleware currently accepts a limit on the maximum size for a single request (say 100MIB). If you spawn lots and lots of concurrent requests, you can easily take up all available memory. As such, this middleware should probably also take care of limiting total memory consumption for all concurrent requests.

Refs #218 Refs #250 Refs #194

clue avatar Nov 21 '17 19:11 clue

Fixing #218 would ideally solve this issue and I rather solve that there then in RequestBodyBufferMiddleware as I'm a) not sure how to solve that in RequestBodyBufferMiddleware and b) I'm afraid we're going to end up with incomplete bodies when we start ignoring chunks.

WyriHaximus avatar Nov 22 '17 16:11 WyriHaximus

I'm currently looking into updating #218 and I agree that this can be used to avoid this issue. However, this will only limit the number of concurrent requests and thus only sets an upper bound limit on the total memory consumption and can not actually take care of actual total memory consumption.

For example, say we use the upcoming RequestLimitHandler set to 100 concurrent requests with each max 10 MB we can take up to 1000 MB max concurrently. On the other hand, if we control this 1000 MB max concurrently in the RequestBodyBufferMiddleware, we can actually process many more requests concurrently as long as they are smaller. Under the reasonable assumption that most requests do not fill this maximum, this allows us to achieve a much better concurrency.

clue avatar Nov 23 '17 10:11 clue

Fair point, but if #218 can be done, we can implement that tech into RequestBodyBufferMiddleware and handle it there as well

WyriHaximus avatar Nov 23 '17 10:11 WyriHaximus

Not sure if that's reasonable, but you're raising a valid point! Arguably, we're discussing a premature optimization here, so I would suggest getting #218 in first and then look into this as a separate feature. Removing the milestone for now :+1:

clue avatar Nov 23 '17 11:11 clue