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

[Andrew Bennetts]

John Arbash Meinel wrote:
> 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

Fixed.

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

Docstrings added.

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

Switched to a comment describing how things are, rather than speculation about
how they might be but aren't :)

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

Right.  Protocol v3 has separate message parts for single bytes (useful for e.g.
status flags), and arbitrary-sized bytestrings (useful for payloads).
Conventional requests never use the single byte message part for anything.
Conventional responses do, though.

> Couldn't a default "read_body_bytes()" be implented in terms of
> read_streamed_body?

You mean in the base ResponseHandler?  That wouldn't be correct, the different
body types are much less interchangeable in the earlier protocol versions.

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

Added to docstring.

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

You've correctly described what happens.  I'm not sure what the third class
you're proposing is, though?

Are you proposing that this be split into a MessageHandler part and
ResponseHandler part?  I'm not sure how that would help.

There is a degree of commonality with what _read_more does and what
_serve_one_request_unguarded on a SmartServerStreamMedium does.  It's probably
possible to add a Medium base class (for all media classes, server and client,
stream and non-stream), and put a _read_more(protocol_decoder) method there.
I'm not sure if it would be significantly better, or if it would just make an
already complicated set of class relationships more complicated.

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

Agreed.  Done.

Incremental diff attached.

-Andrew.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: part-2-update.patch
Type: text/x-diff
Size: 4322 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080430/0bc2c158/attachment-0001.bin