legacysurvey / imagine

The code that runs http://legacysurvey.org/viewer -- map-like viewer for astronomical images, especially the Dark Energy Camera Legacy Survey
BSD 3-Clause "New" or "Revised" License
27 stars 11 forks source link

Limit the size of external uploads (Fixes #42) #45

Closed ziyaointl closed 5 years ago

ziyaointl commented 5 years ago

Looks like the ref to #42 isn't recognized in the title...

ziyaointl commented 5 years ago

This isn't the most straightforward solution, but it's working (sort of). When I tested it on the development server, when the RuntimeError was thrown, used disk size on the server stopped increasing but the client kept uploading. I also tried raising StopUpload(connection_reset=True), but the same thing happens without me being able to catch the exception (Django has caught it instead).

Looks like this is a known bug (that is 10 years old!): https://code.djangoproject.com/ticket/10850

Therefore, it seems to me that limiting file size in the web server config is the easiest solution, though this pull request does provide another safeguard against large files.

dstndstn commented 5 years ago

Hi Mark,

Should we also change the UWSGI setting? Note that we get to choose the uwsgi settings -- in uwsgi.sh and uwsgi-dev.sh -- so should we try setting the "--limit-post" option?

(I guess I might have to do this, because the uwsgi process is running as me on the sgn04 web server.)

--dustin

ziyaointl commented 5 years ago

Yeah changing the uwsgi setting sounds good. If this is sufficient, I’m now tempted to scrape what I was proposing to make sure there’s one source of truth for the max upload setting :) Could you change the setting on the dev server? I can test it afterwards.

On Mon, Jul 15, 2019 at 09:12 Dustin Lang notifications@github.com wrote:

Hi Mark,

Should we also change the UWSGI setting? Note that we get to choose the uwsgi settings -- in uwsgi.sh and uwsgi-dev.sh -- so should we try setting the "--limit-post" option?

(I guess I might have to do this, because the uwsgi process is running as me on the sgn04 web server.)

--dustin

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/legacysurvey/decals-web/pull/45?email_source=notifications&email_token=AEKHTB6RYC3QK7GLGT3NUHTP7SOWBA5CNFSM4ICWK5TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ6F5EA#issuecomment-511467152, or mute the thread https://github.com/notifications/unsubscribe-auth/AEKHTBY62FJEIS6TZEXMH3TP7SOWBANCNFSM4ICWK5TA .

dstndstn commented 5 years ago

Hi Mark,

Sorry for my delay on this -- I just added the --limit-post option and restarted the dev version of the uwsgi server -- would you be able to test?

https://github.com/legacysurvey/decals-web/blob/master/uwsgi-dev.sh#L33

ziyaointl commented 5 years ago

No worries! I tested it out and the options seems to be working. A file of size 40M successfully uploads on the client side and gets passed to Django. A file of size 100M successfully uploads on the client side, but receives a 'Service Temporarily Unavailable' from Apache. A file of size 1000M is stopped at ~35% and receives a 'Service Temporarily Unavailable' from Apache.

The only downside I could imagine to this method is that the user doesn't get a useful output when their legitimate catalog exceeds 50M, but usual catalogs don't go up to this size, so we should be fine?

dstndstn commented 5 years ago

Okay, that seems fine.

On Tue, Jul 16, 2019 at 4:34 PM Ziyao Zhang notifications@github.com wrote:

No worries! I tested it out and the options seems to be working. A file of size 40M successfully uploads on the client side and gets passed to Django. A file of size 100M successfully uploads on the client side, but receives a 'Service Temporarily Unavailable' from Apache. A file of size 1000M is stopped at ~35% and receives a 'Service Temporarily Unavailable' from Apache.

The only downside I could imagine to this method is that the user doesn't get a useful output when their legitimate catalog exceeds 50M, but usual catalogs don't go up to this size, so we should be fine?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/legacysurvey/decals-web/pull/45?email_source=notifications&email_token=AAIEH7MCUXGUWWNNTK2TEZLP7YWFTA5CNFSM4ICWK5TKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2CB5TQ#issuecomment-511975118, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIEH7N3GVZPG2VPOIY3ECLP7YWFTANCNFSM4ICWK5TA .