[Elisa-commits] [MERGE] http_client branch into REST branch, small updates

[Olivier Tilloy]

Yes, there is still (a lot of) space for improvement in this plugin
(which by the way as challenged by Philippe should not be a plugin,
rather a part of the base components or something like that).
But we need it in the rest branch to start working seriously on
developing resource and metadata providers. So my proposition here is to
integrate the small changes requested here by Benjamin, commit and merge
into rest, and then the bigger changes, like adding test cases, can be
done later (no I'm not skipping them).
Anyone opposed to this, Speak Now or Forever Hold Your Peace!

Olivier

Benjamin Kampmann wrote:
> bb:tweak
> 
> I assume that the externals are good so far. Did not understand everything
> but did a quick overview and there was nothing that looked wrong (in
> term of
> syntax, naming or similar).
> 
> I did a deeper look into the part that is written by us and I have some
> questions that
> came up while reviewing. I think none of them is a show stopper so far
> but we also want
> clean and good maintainable good, right ;) .
> 
> here we go.
> Benjamin
> 
> === added file 'elisa-plugins/elisa/plugins/http_plugin/http_client.py'
> --- elisa-plugins/elisa/plugins/http_plugin/http_client.py     
> 1970-01-01 00:00:00 +0000
> +++ elisa-plugins/elisa/plugins/http_plugin/http_client.py     
> 2008-04-14 14:25:00 +0000
> @@ -0,0 +1,356 @@
>    [...]
>> +class ElisaHttpClientFactory(ReconnectingClientFactory):
>> +    # after 3 retries we give up
>> +    maxRetries = 3
>> +
>> +    def __init__(self, client):
>> +        self.client = client
>> +
>> +        # base has no init
>> +
>> +    def buildProtocol(self, addr):
>> +#        self.resetDelay()
> What is this about? If we outcomment it, we could also remove it, or?

No idea. Alessandro, any comment on this?

>> +        protocol = ElisaHttpClientProtocol(self.client)
>> +        +        # FIXME: check the spec, probably we shouldn't call
>> this and wait for a
>> +        # Connection: Keep-Alive header from the server
>> +        persist = None
>> +        if self.client.pipeline:
>> +            persist = PERSIST_PIPELINE
>> +        else:
>> +            persist = PERSIST_NO_PIPELINE
>> +        protocol.setReadPersistent(persist)
>> +
>> +        return protocol
> 
> Well, the specs ( http://www.ietf.org/rfc/rfc2616.txt ) says this:
> 
> ---- snip ----
> 
> 8.1.2 Overall Operation
> 
> A significant difference between HTTP/1.1 and earlier versions of
> HTTP is that persistent connections are the default behavior of any
> HTTP connection. That is, unless otherwise indicated, the client
> SHOULD assume that the server will maintain a persistent connection,
> even after error responses from the server.
> 
>    [...]
> 
> 8.1.2.1 Negotiation
> 
>    [...]
> 
> An HTTP/1.1 server MAY assume that a HTTP/1.1 client intends to
> maintain a persistent connection unless a Connection header including
> the connection-token "close" was sent in the request.
> 
>    [...]
> 
> Clients and servers SHOULD NOT assume that a persistent connection is
> maintained for HTTP versions less than 1.1 unless it is explicitly
> signaled. See section 19.6.2 for more information on backward
> compatibility with HTTP/1.0 clients.
> 
> ---- snip ----
> 
> So assuming is alright as long as the server speaks HTTP/1.1 . I dunno
> enough
> to say if this is possible at this point or not, but we should definitly
> look
> forward to implement the specs correctly. For now, I would say, let's
> change
> the FIXME to this:
> 
>    FIXME: according to the specs, we are only allowed to assume a
> persistent
>    connection in HTTP/1.1. Check if we got a 'Connection: Keep-Alive' or
>    HTTP/1.1 (and no connection information) is in the Header is necessary.
> 
> Maybe we should also add a TestCase for it even if it fails atm (see
> above) to
> not forget about that.

Will change the FIXME now and add the TestCase later.

>    [...]
>> +
>> +    def startedConnecting(self, connector):
>> +        # log me
>> +        pass
> Okay. Who logs here?

Good question. Alessandro, are you logging?

>    [...]
> 
>> +class ElisaHttpClient(Loggable):
>> +    """twisted.web2 based HTTP client."""
>    [...]
>> +
>> +    def open(self, host, port=80):
>> +        """Open an HTTP connection.
>> +
>> +        @param host:        hostname or ip address of the server
>> +        @type host:         str
>> +        @param port:        tcp port
>> +        @type port:         int
>> +
>> +        """
>> +        self._host = host
>> +        self._port = port
>> +        self._closed = False
>> +        self._connector = reactor.connectTCP(host, port,
>> +                ElisaHttpClientFactory(self))
>> +
>> +        return self._open_defer
>> +
>> +    def request(self, uri, method='GET'):
>> +        return self.request_full(ClientRequest(method, uri, {}, None))
>> +
>> +    def request_full(self, request):
>> +        """Send an HTTP request.
>> +
>> +        @param request:     request to submit
>> +        @type request:      L{twisted.web2.client.http.ClientRequest}
>> +        @rtype:             L{twisted.internet.defer.Deferred}
>> +        """
>> +
>> +        if self._closed:
>> +            return defer.fail(ElisaHttpClientNotOpenedError())
> As we just set self._closed to False in open (above) can we be sure by just
> checking here that we have a connection established? Shouldn't we attach a
> callback to self._open_defer that changes self._closed to False/True
> when ever
> there is a change?
> We should also add a comment in the docstring here saying that you need to
> wait until the defer of open got fired before sending a request (and
> maybe add
> some docstring to request, too ;) )

Agreed, changing this.

>    [...]
>> +    def _queue_request(self, request):
>> +        res = None
>> +        busy = False
>> +        if self.pipeline:
>> +            busy = bool(self._queued_requests)
>> +            request.defer = defer.Deferred()
>> +            self._queued_requests.append(request)
>> +        else:
>> +            busy = bool(self._queued_requests or self._written_requests)
>> +            if busy:
>> +                res = defer.fail(ElisaHttpClientNotPipeliningError())
> Couldn't we just return this here? the variable *res* is very useless, or?

Mmm I thought we were supposed to avoid having more than one return
statement per method, so my guess is that the original goal was to
enforce this policy, but it evidently fails to do so, so I'm removing
this res crap.

>    [..]
>> +class HttpResourceClient(ElisaHttpClient):
>> +
>> +    """
>> +    An ElisaHttpClient specialization that handles HTTP redirections.
>> +    """
> The name of the class does make clear that this for handling redirections
> correctly. What is so specialized to (Elisa) Resources about this client?

does or does not? The only justification I can give to support that name
is that it's the client that resource providers should use... But yeah,
I was not in a good name-picking mood when I wrote this, what about
ElisaAdvancedHttpClient?

> === added directory 'elisa-plugins/elisa/plugins/http_plugin/tests'
> === added file 'elisa-plugins/elisa/plugins/http_plugin/tests/__init__.py'
> === added file
> 'elisa-plugins/elisa/plugins/http_plugin/tests/test_http_client.py'
> --- elisa-plugins/elisa/plugins/http_plugin/tests/test_http_client.py  
> 1970-01-01 00:00:00 +0000
> +++ elisa-plugins/elisa/plugins/http_plugin/tests/test_http_client.py  
> 2008-04-14 14:25:00 +0000
> @@ -0,0 +1,605 @@
>    [...]
>> +from zope.interface import implements
> Question: Does elisa (or twisted) already have a strong dependency on
> zope(.interfaces) or do we need to add this to the README?

apt-cache rdepends python-zopeinterface
...
python-twisted-core has a dependency on python-zopeinterface, no need to
modify the README.

>> +class TestElisaHttpClientPipelining(ElisaTestCase, SetupServerMixin):
>> +
>> +    """
>> +    This test case tests the ElisaHttpClient class with pipelining.
>> +    """
>    [...]
>> +    def test_response_contents(self):
>> +        """
>> +        Test the actual contents of the response returned by the server.
>> +        """
>> +        resource = 'foo'
>> +        result_dfr = defer.Deferred()
>> +
>> +        def build_response_string(resource):
>> +            title = 'Elisa test HTTP Server BETA'
>> +            content = resource * 10
>> +            response =
>> '<html><head><title>%s</title></head><body><h1>%s</h1><p>%s</p></body></html>'
>> % (title, title, content)
> We usually have a 78 colums wrap coding style... Please change that..

Will do. BTW, why 78 and not 80?

>    [...]
>> +class TestHttpResourceClientRedirections(ElisaTestCase,
>> SetupServerMixin):
>> +
>> +    """
>> +    This test case tests the ElisaHttpClient class with pipelining.
>> +    """
> the same docstring as the class above? I would guess this one is in fact
> about
> redirections ;)

Let there be copy-pasting! Good guess...

> Adding a test about the redirections loop (ref to the FIXME in the code)
> would
> be good. Even if it fails now, it makes us not forget about to implement
> this
> behaviour later. The same for the HTTP/1.1 - Connection-Keep-Alive thing.
> Adding the tests now even if they fail is good thing IMHO.

Will do that in phase 2, right after the merge.

> This is no show-stopper so far, as we decided to first merge and then redo
> some of the tests anyway.