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

Correct way to set aiohttp's client_max_size ? #222

Closed
SmithChart opened this issue May 21, 2022 · 4 comments · Fixed by #224
Closed

Correct way to set aiohttp's client_max_size ? #222

SmithChart opened this issue May 21, 2022 · 4 comments · Fixed by #224

Comments

@SmithChart
Copy link
Contributor

I am working on an application where I want to upload files using POST. But files can exceed aiohttp's default client_max_size (https://docs.aiohttp.org/en/stable/web_reference.html#aiohttp.web.Application ) limit.

I am running my server from the command line (aka the Makefile). So the right place to set this limit would be here: https://github.com/lona-web-org/lona/blob/master/lona/command_line/run_server.py#L29

What do you think should be the correct way to set this limit? Add another command line option? Any opinions?

@fscherf
Copy link
Member

fscherf commented May 22, 2022

Hi @SmithChart!

I think a settings variable would be the best way to implement this. This way it can be set by the project and by the command via -O $SETTING=$VALUE.

AIOHTTP_CLIENT_MAX_SIZE maybe?

@SmithChart
Copy link
Contributor Author

Hi @fscherf,
AIOHTTP_CLIENT_MAX_SIZE sounds good. I like the idea to have that flexibility.

The problem with using the settings (at least as far as I can see) is that settings are loaded by the LonaServer but at this point the aiohttp Application has already been created and the size limit is already set.
It seems there is a way to override this limit on a per-request base using a aiohttp middleware (aio-libs/aiohttp#5704 ). But having a middleware for this seems overkill. Do you see a way to load the settings before creating the aiohttp Application?

On a sidenote:
I wasn't aware that there is a way to set settings via the command line. Maybe we should add a note to the settings-documentation (https://lona-web.org/end-user-documentation/settings.html )?

@fscherf
Copy link
Member

fscherf commented May 23, 2022

@SmithChart:

  1. Can we set the value in the aiohttp app when the server first starts? In this case we could do that in the LonaServer constructor.

  2. Right! Currently it's only mentioned in the debbuging and script sections.

@SmithChart
Copy link
Contributor Author

1. Can we set the value in the aiohttp app when the server first starts? In this case we could do that in the `LonaServer` constructor.

Yes, that's possible. My hack currently looks like:

diff --git a/lona/command_line/run_server.py b/lona/command_line/run_server.py
index 69d8134..6430de4 100644
--- a/lona/command_line/run_server.py
+++ b/lona/command_line/run_server.py
@@ -26,7 +26,7 @@ def run_server(args, app=None, server=None):
     log_formatter, log_filter = setup_logging(args)
 
     # setup lona server
-    app = app or Application()
+    app = app or Application(client_max_size=1024*1024*100)

I'll prepare a PR. That makes discussion easier.

2. Right! Currently it's only mentioned in the debbuging and script sections.

👍

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

Successfully merging a pull request may close this issue.

2 participants