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

add Expect: 100-continue handling for #267 #268

Closed
wants to merge 10 commits into from

Conversation

tumb1er
Copy link
Contributor

@tumb1er tumb1er commented Feb 2, 2015

  • if handler yields from request.post(), it actually agrees with "100-continue" expectation, so server must send "HTTP/1.1 100 Continue"
  • if handler doesn't agree, it sends Response without reading whole request body (it may read, but don't need this)

So, request.post() is a place where 100-Continue answer could be safely sent to HTTP/1.1 clients.

see #267 for more defails

@tumb1er tumb1er changed the title add Expect: 100-continue handling for #263 add Expect: 100-continue handling for #267 Feb 2, 2015
@kxepal
Copy link
Member

kxepal commented Feb 2, 2015

Could you please rebase your branch to exclude merge commits and squash #263... ones? No need to pollute change history when you can avoid that (that's about eaf724c 9e17320 and 3bea111 changes). Tests and implementation should be done in single commit since they're logically belongs to the single feature.

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 2, 2015

i agree "continue" should be default behavior, but there are several issues:

  • it may be possible to use "expect" on non post requests, so i don think post() is good place for that.
  • and i don't see how developer can change behavior of "continue"

@tumb1er
Copy link
Contributor Author

tumb1er commented Feb 3, 2015

@kxepal I'd rather create a new pull request.
@fafhrd91

  • yes, and nginx server always sends 100-continue (because it checks "expectations" by itself). post() is good, but not optimal place.
  • i.e.:
  1. Client sends huge POST-request with "Content-Length: 100500900" and "Content-Type": "application/octet-stream".
  2. It also sends "Expect: 100-continue" and waits for server expectation check.
  3. Server (aiohttp) receives and parses request headers, and send it to handler
  4. Handler before yielding post() checks content-length and just returns "HTTPRequestEntityTooLarge". No payload data need to be ever read, so handler just returns and that's all.
  5. Another case: handler permits this huge request and yields post(). Aiohttp server performs "content-type" check and just returns {} as parsed request. In this PR 100-continue is not sent, because post() actually don't read request payload. If handler wants to read octet-stream, it will be forced to read request.content and handle it by itself (and in this case developer MUST send 100-continue by himself - it's bad).
  6. Third case: handler says 'ok', request sends 100-continue and parses "multipart/form-data" payload - it's normal behavior.

I just don't see how developer may want to change "100-continue" behavior and have unsolvable problems with it at same time.

All in all, this PR:

  • there is a problem with handling 100-continue for GET/HEAD/OPTIONS - requests without payload. It's stupid, but client wants to see "100-continue" before not-sending-missing-payload and finalizing http request.
  • problem with handling "octet-stream" and other CT, that are not handled with Request.post.
  • some questions about usability of forcing to send or not to send 100-continue header.

It might be better to place all these into a separate base middleware, somethink like this:

class ExpectContinueMiddlewareBase(object):
  def factory(cls):
    pass

  def check_expectation(self, request):
    """ if expectation is not met, returns final Response for it.
    Returns none if all is ok. """
    raise NotImplementedError()

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 4, 2015

I don't like this implementation, it is too specific.

What if we add Request.continue() or it could be route object responsibility.

@fafhrd91
Copy link
Member

fafhrd91 commented Feb 4, 2015

Making route responsible for continue is cleanest solution, but I don't like it right now. @asvetlov thoughts?

@fafhrd91
Copy link
Member

close #287

@fafhrd91 fafhrd91 closed this Mar 11, 2015
@tumb1er tumb1er deleted the expect branch April 7, 2017 08:45
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants