Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequestBodyBufferMiddleware should limit total memory consumption for concurrent requests #255

Open
clue opened this issue Nov 21, 2017 · 4 comments

Comments

@clue
Copy link
Member

clue commented Nov 21, 2017

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 clue added this to the v0.8.0 milestone Nov 21, 2017
@WyriHaximus
Copy link
Member

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.

@clue
Copy link
Member Author

clue commented Nov 23, 2017

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.

@WyriHaximus
Copy link
Member

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

@clue
Copy link
Member Author

clue commented Nov 23, 2017

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants