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

[Benjamin Kampmann]

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?

>+        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.

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

    [...]

>+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 ;) )

    [...]
>+    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?


    [..]
>+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?


=== 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?


>+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..


    [...]
>+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 ;)

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.

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