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

sanic/app: When streaming, check REQUEST_MAX_SIZE if length is known #2745

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ozancaglayan
Copy link

Following the discussion, this PR adds a check for the streaming case to early quit request processing in case request has a content length larger than the REQUEST_MAX_SIZE. Tested locally and it seems to work.

@ozancaglayan ozancaglayan requested a review from a team as a code owner April 25, 2023 23:12
@Tronic
Copy link
Member

Tronic commented Apr 27, 2023

This appears to change functionality such that streaming requests are no longer of infinite size (which is intended functionality).

Can you accomplish the same if you raise that exception in your handler? Also, which client are you testing with? Curl and possibly httpx (the test client) behave differently of browsers because they use expect: 100-continue mechanism that allows rejecting the request without receiving it; possibly this is what you accomplish with this PR, but browsers don't send that header and thus won't be "fixed" if that is the case.

My comments are a bit fuzzy - sorry about that - because I don't have time to dig into the code and run my own tests on this right now.

@ahopkins
Copy link
Member

ahopkins commented Jul 9, 2023

I thought this would only be something to take into account if there is a known length, and not just any streaming request.

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 this pull request may close these issues.

None yet

3 participants