[MERGE] Add bzrlib.smart.message module: new abstractions for protocol logic (protocol v3 patch 2/7)

[John Arbash Meinel]

John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
+++ bzrlib/smart/message.py     2008-04-23 10:57:12 +0000
...

+from bzrlib import (
+    debug,
+    errors,
+    )
+from bzrlib.trace import mutter
+
+class MessageHandler(object):

^- two spaces here


As MessageHandler seems to be an important abstract class, it really 
needs
docstrings. As is, only 'protocol_error' has a docstring.

ConventionalRequestHandler also needs doc strings. I don't know that 
every
function needs one, but the class in general does (it might just defer 
to the
other document, but we should have a reference here.)


+    def bytes_part_received(self, bytes):
+        # XXX: this API requires monolithic bodies to be buffered
+        # XXX: how to distinguish between a monolithic body and a chunk 
stream?
+        #      Hmm, I guess the request handler knows which it is 
expecting
+        #      (once the args have been received), so it should just 
deal?  We
+        #      don't yet have requests that expect a stream anyway.
+        #      *Maybe* a one-byte 'c' or 'm' (chunk or monolithic) flag 
before
+        #      first bytes part?
+        self.request_handler.accept_body(bytes)
+        self.request_handler.end_of_body()
+        assert self.request_handler.finished_reading
+        self.responder.send_response(self.request_handler.response)

^- Generally, I feel like XXX shouldn't be merged into core. Either it 
is
important, or it is just a TODO.

I don't quite understand why you have "byte_part_received" separate from
"bytes_part_received" and why for a "ConventionalRequestHandler"
byte_part_received is always an exception.

Maybe this is just the protocol description. (BYTE, STRUCTURE, BYTES?)


Couldn't a default "read_body_bytes()" be implented in terms of
read_streamed_body?
You don't seem to document that "count=-1" means read the whole body. 
Just that
if nothing is supplied, read the whole body. (That could be implemented 
as
count=-1, or count=None, so you should be clear here.)


It seems a little odd to me that ConventionalResponseHandler has 
"_read_more"
as a member.
Specifically, you have CRH consuming data from the socket, and passing 
it to
self._protocl_decoder.accept_bytes(), which seems like it would then 
call back
into CRH.byte/bytes/structure_part_received(). It just feels to me like 
this
should be 3 classes, not 2.


+    def read_body_bytes(self, count=-1):
+        """Read bytes from the body, decoding into a byte stream.
+
+        We read all bytes at once to ensure we've checked the trailer 
for
+        errors, and then feed the buffer back as read_body_bytes is 
called.
+        """
+        # XXX: don't buffer the full request

^- I would probably consider this a TODO. And it should probably be 
dated, and
have your name.




For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C20080423225406.GF9546%40steerpike.home.puzzling.org%3E