Opened 8 years ago

Last modified 5 years ago

#6928 enhancement new

Implement HTTP request acception / rejection as defined in RFC 2616 - 8.2.3

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight, lukasaoz@… Branch:
Author:

Description (last modified by Jean-Paul Calderone)

Right now HTTPRequest will accept all request asking for accept: 100-continue

In my view it would be nice to let the resource to decide whether or not it wants to accept the request.

This might be a step toward fixing #288 as it define a way for resource to notify the resource before body is received.

Link to RFC http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.3

Error handling: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.20

Change History (11)

comment:1 Changed 8 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 8 years ago by Jean-Paul Calderone

Description: modified (diff)

comment:2 Changed 8 years ago by Adi Roiban

Summary: Implement HTTP request acception / rejection as defined in RFC 2616 Sectin 8.2.3Implement HTTP request acception / rejection as defined in RFC 2616 - 8.2.3

comment:3 Changed 8 years ago by Jean-Paul Calderone

#4673 suggests #288 is actually a dependency of this ticket, not the other way around.

That makes sense to me. What API will this use to ask a resource if it wants to accept the request or not? It will presumably require a new interface and a change to traversal to find the resource before the request has been completely received.

comment:4 Changed 8 years ago by Adi Roiban

I have implemented a prototype using something similar to your sketch from #288.

I have extended the IRequest interface with a headersReceived(request) method.

HTTPChannel will also call Request.headersReceived()

Does it make sense to you?

Here is a preview. I am cleaning it up and will put it in a branch.


class Request:

    def headersReceived(self, command, path, version):
        """
        Called by channel when all headers has been received.
        """
        SOME_SETUP_CODE similar to contentReceived setup

        # resource is cached so we do a single traversal.
        self._resource = self.site.getResourceFor(self)
        result = self._resource.headersReceived(self)
        self._processHeadersReceived(result)

    def _processHeadersReceived(self, result):
        """
        Continue request process based on response from
        resource.headersReceived()
        """
        self.transport.resumeProducing()
        # Handle HTTP/1.1 'Expect: 100-continue' based on RFC 2616 8.2.3.
        expectContinue = self.requestHeaders.getRawHeaders('expect')
        if self.clientproto == 'HTTP/1.1' and expectContinue:
            if expectContinue[0].lower() != '100-continue':
                self.code = http.EXPECTATION_FAILED
                self.finish()
                return

            if result == http._CONTINUE:
                self.transport.write("HTTP/1.1 100 Continue\r\n\r\n")
            else:
                self.code = result
                self.finish()

comment:5 Changed 8 years ago by Jean-Paul Calderone

There is an additional challenge. The resources traversed by self.site.getResourceFor(self) may not be prepared to deal with a request that is only partially received. For example, they might try to use request.content to inspect the body.

I think the conclusion reached for #288 is that this needs to be an opt-in feature (opting in probably means declaring you implement a new interface). Traversal aimed at implementing this feature needs to stop as soon as it encounters a resource that hasn't opted in (thus disabling the new feature for that resource and all its children).

comment:6 Changed 8 years ago by Jean-Paul Calderone

I think a frequent stumbling block to improving Twisted Web has been a fear that if we change any of the core interfaces we had better change them *a ton* because there's so much wrong with all of them.

To avoid being derailed by that issue yet again, I suggest this simple interface versioning scheme.

Interfaces should have version = Attribute("..."). Implementations should declare which version they are actually implementing. Other attributes and methods on the interface should declare which version they were added.

Code that uses providers of these interfaces should check the version the provider declares and make the appropriate decisions.

This means we can introduce new features to IResource, keep the name, and gracefully fall back when handling older implementations of IResource. The "first" version of the interface can be numbered 1 but since IResource existed before the code will still need to handle the version attribute being missing (representing a "version zero" provider) when using this for feature checking.

comment:7 Changed 8 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to Jean-Paul Calderone

I have not attached a patch file since this code is not yet ready for merge.

A web friendly diff is here: https://github.com/chevah/twisted/compare/6928-http-100-accept

A diff file is here: https://github.com/chevah/twisted/compare/6928-http-100-accept.diff

Please not that this is only a prototype code which was copied from a dirty implementation. The code does not work.

Once the general API and approach is decided I will write test for it and write the required documentation.

The proposed interfaces has no version Attribute since I wanted to keep thinks simple for an initial review.

@exarkun I think the conclusion reached for #288 is that this needs to be an opt-in Traversal aimed at implementing this feature needs to stop as soon as it encounters a resource that hasn't opted in (thus disabling the new feature for that resource and all its children).

I am not sure how an opt-in implementation should look like. I tried my luck.

Please check the code marked with FIXME. I don't know how the HTTPChannel.requests lists should be used.

Right now Resource.headersReceived() is only use to check whether or not the request processing should continue. In the future, to help with bug #288 I am thinking to use the same method to also allow the resource to register a body consumer.

Not sure if the new interfaces should be private or not.

Resource.requestReceive() has the same signature, since I wanted to minimize code changes. I think that the cleanup should be done in a separate ticket. Most of the code is done now in Resource.headersReceived

I assume that this branch also needs to touch the documentation and examples.

Calls to Request.parseCookies() Request.gotLength(self.channel.length) were moved inside Request class. Maybe we can make them private.

Code from Request.process and Request.contentReceived was moved into the new Request.headersReceived.

I don't know how to break this branch as the diff is already at 300 lines and no tests were touched.

Resource._setResourceIndentification is almost good, but it also touches the self.args which has nothing to do with URI/Path.

In my original prototype I have also implemented IEaryHeaders versions for twisted.web.util.DeferredResource twisted.web.guard.HTTPAuthSessionWrapper. I guess that is better to have a separate ticket and review for those changes.

This is why the code also handlers deferred returned Resource.headersReceived. Please let me know if you think that we should not add initial support for deferreds returned by Resource.headersReceived.

@exarkun, I have assigned this ticket to you, but feel free to remove the owner if you don't have time to review it.

Thanks!

comment:8 in reply to:  7 Changed 8 years ago by Glyph

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Adi Roiban

Replying to adiroiban:

I have not attached a patch file since this code is not yet ready for merge. Once the general API and approach is decided I will write test for it and write the required documentation.

Hi adiroiban,

It sounds like what you want here is a general discussion of implementation approaches, not a code review.

I think this would be better suited to the mailing list, because there's a lot to talk about here, and the trac comment box is small.

Also, starting with docs and tests can often reveal a lot of issues that just trying to monkey with implementation might not. I would suggest doing a fully test-driven attempt at this and see what stumbling blocks you run into.

This does seem like a promising direction but I'm going to take it out of the review queue to make room for things that are actually ready for full review / landing. Please raise this on the twisted-web list so we can talk about how best to implement it.

comment:9 Changed 6 years ago by Cory Benfield

Cc: lukasaoz@… added

comment:10 Changed 5 years ago by Adi Roiban

Last edited 5 years ago by Adi Roiban (previous) (diff)
Note: See TracTickets for help on using tickets.