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

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
| 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.

...

My problem is that it feels like you can have sections coming in at any time
(just by how things are named, etc) but you only really support them coming in
as BYTE, STRUCTURE, BYTES. It also seems that BYTE and STRUCTURE can only exist
one time, but BYTES can be present multiple times?

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

...

Having a class call functions in another class that cause callbacks creates a
loop, which I personally find harder to understand than a linear call chain. If
they are actually interdependent, that is a bit different. But for
ConventionalResponseHandler, I'm pretty sure it is mostly "open loop". Parts of
data either exist or don't, but the next request doesn't depend much on the
previous one.

Anyway, I'm keen to get your code into play, just saying that the loop seems a
bit wrong. (Not to mention it creates a cycle for the python GC).

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgY4o8ACgkQJdeBCYSNAAMs6gCfUEM2laaSr1mbk4tFDiR1zvJb
L1UAniKAFVNKcpNNM/a+FSw5QX7yk8u/
=N0za
-----END PGP SIGNATURE-----