[merge] review of overall protocol3 diff

[Andrew Bennetts]

Martin Pool wrote:
> I thought I would review the overall hpss patch, to see things that
> changed based on previous reviews and just as an extra check.
> 
> Some of these things would be good to do soon, but don't need to block
> landing so I've marked them with ZZ.
> 
> Unfortunately I haven't finished this yet, it's very big, but here are
> some comments on the
> first part.

Thanks!

[...]
> -            # version (2) so that the VFS-based transport will do it.
> +            # version (3) so that the VFS-based transport will do it.
>              try:
>                  server_version = medium.protocol_version()
>              except errors.SmartProtocolError:
>                  # Apparently there's no usable smart server there, even though
>                  # the medium supports the smart protocol.
>                  raise errors.NotBranchError(path=transport.base)
> -            if server_version != 2:
> +            if server_version not in (2, 3):
>                  raise errors.NotBranchError(path=transport.base)
>              return klass()
> 
> 
> I think we said the other day we're going to drop this, and if the transport
> can do RPCs at all, we'll just assume that.  However, that's not quite enough
> for HTTP, where it's theoretically capable of doing RPC but there may not
> actually be a server there...

Right.  A patch to do just that has been posted and reviewed separately.

> === modified file 'bzrlib/errors.py'
> --- bzrlib/errors.py	2008-04-24 16:44:23 +0000
> +++ bzrlib/errors.py	2008-05-08 05:49:30 +0000
> @@ -1476,6 +1476,14 @@
>          self.details = details
> 
> 
> +class UnexpectedProtocolVersionMarker(TransportError):
> +
> +    _fmt = "Unexpected protocol version marker: %(marker)r"
> +
> +    def __init__(self, marker):
> +        self.marker = marker
> +
> +
>  class UnknownSmartMethod(InternalBzrError):
> 
> Does this mean "it's definitely a marker but we didn't expect it at this time"
> or "we don't understand the protocol our counterparty is trying to
> use?"  If the
> latter maybe it can have a clearer message.

It's used for both “it's a marker but not the one I'm looking for” (i.e. the
response protocol doesn't match the request protocol) and “we don't understand
the protocol the peer is using”.  But mainly the latter in practice, I think.
The former is theoretically possible, but I don't expect it to happen at the
moment.

The current message is ambigous about what exactly is unexpected: is it the fact
we received a marker now, or the contents of the marker itself?  (The answer is
always the latter.)  I've changed it to “Received bad protocol version marker:
%(marker)r”, which I hope resolves that ambiguity.

[...]
>      def get_branch_reference(self):
>          """See BzrDir.get_branch_reference()."""
>          path = self._path_for_remote_call(self._client)
> -        response = self._client.call('BzrDir.open_branch', path)
> +        try:
> +            response = self._client.call('BzrDir.open_branch', path)
> +        except errors.ErrorFromSmartServer, err:
> +            if err.error_tuple == ('nobranch',):
> +                raise errors.NotBranchError(path=self.root_transport.base)
> +            raise
>          if response[0] == 'ok':
>              if response[1] == '':
>                  # branch at this location.
> 
> This is a nice change, that these errors can now all be treated as
> client side errors by default.

Thanks, I'm glad someone other than me likes it :)

> ZZ: I do wonder though whether we should take this a step further:
> isn't it always appropriate to turn this into NotBranchError in e.g.
> _raise_args_if_error?

As you notice below, this isn't trivial.  It would be nice to do, though.

[...]
> -            response = self._client.call(verb, path)
> -        except errors.UnknownSmartMethod:
> -            verb = 'BzrDir.find_repository'
> -            response = self._client.call(verb, path)
> -        assert response[0] in ('ok', 'norepository'), \
> -            'unexpected response code %s' % (response,)
> -        if response[0] == 'norepository':
> -            raise errors.NoRepositoryPresent(self)
> +            try:
> +                response = self._client.call(verb, path)
> +            except errors.UnknownSmartMethod:
> +                verb = 'BzrDir.find_repository'
> +                response = self._client.call(verb, path)
> +        except errors.ErrorFromSmartServer, err:
> +            if err.error_tuple[0] == 'norepository':
> +                raise errors.NoRepositoryPresent(self)
> +            raise
> +        if response[0] != 'ok':
> +            raise errors.UnexpectedSmartServerResponse(response)
>          if verb == 'BzrDir.find_repository':
>              # servers that don't support the V2 method don't support external
>              # references either.
> 
> ZZ: Again, it seems like many commands repeat this check for 'ok' and
> raising an error
> otherwise, surely it can be factored out?

Well, valid responses vary by request; “ok” is only valid for some.  So it's not
immediately obvious how best to factor this.

> @@ -372,26 +374,25 @@
> 
>          path = self.bzrdir._path_for_remote_call(self._client)
>          assert type(revision_id) is str
> -        response = self._client.call_expecting_body(
> -            'Repository.get_revision_graph', path, revision_id)
> -        if response[0][0] not in ['ok', 'nosuchrevision']:
> +        try:
> +            response = self._client.call_expecting_body(
> +                'Repository.get_revision_graph', path, revision_id)
> +        except errors.ErrorFromSmartServer, err:
> +            if err.error_tuple[0] == 'nosuchrevision':
> +                raise NoSuchRevision(self, revision_id)
> +        if response[0][0] != 'ok':
> 
> You are swallowing the error if it is not nosuchrevision.  Rather than getting
> every one of these methods right (or wrong) individually I'd really like to
> centralize it.

And as John noticed, it's in fact buggy if the error is not nosuchrevision
(response will be undefined).  There's now a bare “raise” fallback here, and a
test.

I would love to find a nice way to handle this centrally.

[...]
> Just as a point of style (not for now) I think code like
> 
>    coded = response[1].read_body_bytes()
> 
> would be much better as
> 
>    status, response = blah._call()
>    ....
>    coded = response.read()
> 
> in other words avoiding subscripting on tuples that can sensibly be given names.

You're right.  I've gone ahead and done this now (it doesn't take that long, and
it's especially nice to lose the double subscripts like “[0][0]”).

> @@ -1519,10 +1534,11 @@
>              self._ensure_real()
>              self._clear_cached_state()
>              return self._real_branch.set_last_revision_info(revno, revision_id)
> +        except errors.ErrorFromSmartServer, err:
> +            if err.error_tuple[0] == 'NoSuchRevision':
> +                raise NoSuchRevision(self, err.error_tuple[1])
> 
> And here.

This has been improved by adding “error_verb” and “error_args” attributes to
ErrorFromSmartServer (calculated from the existing “error_tuple”).

> +    def _call_and_read_response(self, method, args, body=None, readv_body=None,
[...]
> +                try:
> +                    response_tuple = response_handler.read_response_tuple(
> +                        expect_body=expect_response_body)
> +                except errors.UnexpectedProtocolVersionMarker, err:
> +                    # TODO: We could recover from this without disconnecting if
> +                    # we recognise the protocol version.
> +                    self._medium.disconnect()
[...]
> 
> We should warn that the server is old so we're reconnecting.

Done:

                    warning(
                        'Server does not understand Bazaar network protocol %d,'
                        ' reconnecting.  (Upgrade the server to avoid this.)')

> @@ -202,9 +231,6 @@
>          return self.socket.recv(4096)
> 
>      def terminate_due_to_error(self):
> -        """Called when an unhandled exception from the protocol occurs."""
> -        # TODO: This should log to a server log file, but no such thing
> -        # exists yet.  Andrew Bennetts 2006-09-29.
>          self.socket.close()
>          self.finished = True
> 
> Is this done now?  Why remove the comment?

It isn't.  I've no idea why I removed that comment.  I've reinstated it.

> === added file 'bzrlib/smart/message.py'
> --- bzrlib/smart/message.py	1970-01-01 00:00:00 +0000
> +++ bzrlib/smart/message.py	2008-05-08 05:49:31 +0000
> 
> +class ConventionalRequestHandler(MessageHandler):
> +    """A message handler for "conventional" requests.
> +
> +    "Conventional" is used in the sense described in
> +    doc/developers/network-protocol.txt: a simple message with arguments and an
> +    optional body.
> +    """
> +
> +    def __init__(self, request_handler, responder):
> +        MessageHandler.__init__(self)
> +        self.request_handler = request_handler
> +        self.responder = responder
> +        self.args_received = False
> 
> There is some naming confusion about a class called ConventionalRequestHandler
> that has a member called request_handler that is not of the same class.  Maybe
> this should be called ConventionalMessageHandler?

Yeah, I think the root problem here is bzrlib.smart.request's
SmartServerRequestHandler class, which is a bit misnamed.  That's what the
“request_handler” here is, but really that object is responsible for dispatching
to the right SmartServerRequest class rather than “handling” anything itself.
The knock-on effects of renaming it are a bit too large for me to want to tackle
in this patch, though.

I don't think ConventionalRequestHandler would be better called
ConventionalMessageHandler.  That name could equally well apply to
ConventionalResponseHandler!

> Since the messages are symmetric it would be nice to unify these classes.  Or
> at least have a common base class that does all the network-facing stuff.

These class have a MessageHandler base class already, and all the network-facing
stuff is already common (that's what ProtocolThreeDecoder/Encoder do).  So I'm
not sure what you're suggesting I change here?

> +
> +    def protocol_error(self, exception):
> +        self.responder.send_error(exception)
> +
> +    def byte_part_received(self, byte):
> +        raise errors.SmartProtocolError(
> +            'Unexpected message part: byte(%r)' % (byte,))
> 
> I'm confused about why this is not accepting byte parts, shouldn't it
> take 'E', 'S', etc,
> similar to the ResponseHandler?

Well, there's no byte part defined for conventional requests in the protocol
spec.  In responses, it's used as a flag to say if the response is an error or a
success, but obviously that doesn't apply to requests.

[...]
> +    def bytes_part_received(self, bytes):
> +        # Note that there's no intrinsic way to distinguish a monolithic body
> +        # from a chunk stream.  A request handler knows which it is expecting
> +        # (once the args have been received), so it should be able to do the
> +        # right thing.
> +        self.request_handler.accept_body(bytes)
> +        self.request_handler.end_of_body()
> +        if not self.request_handler.finished_reading:
> +            raise SmartProtocolError(
> +                "Conventional request body was received, but request handler "
> +                "has not finished reading.")
> +        self.responder.send_response(self.request_handler.response)
> 
> I don't understand how a streamed body fits in here.  Wouldn't that be
> multiple bytes parts?
> 
> It would be nice to add that now so that we can stream requests without
> worrying whether the server is so old that it will reject them, and without it
> needing to use different RequestHandlers for different verbs.
> 
> Surely this should be split so that it keeps just giving bits of the body to
> the request_handler as they come in, then only when it gets the end of the
> message it says it's finished?
> 
> Now it's true we don't use this at the moment, but I'm concerned if we don't
> fix this before landing this protocol it will be another case of causing
> problem with forward compatibility.

As discussed on the phone, there's no request with a streaming body at the
moment.  I think it can be accomodated in the existing code and protocol.  So
the plan is to add a request that uses a streaming body immediately after
landing this, to shake out any wrong assumptions I have, and make sure v3 can
handle streaming request bodies (and if not, make any necessary changes to it
before we make a release including it).

[...]
> +class ConventionalResponseHandler(MessageHandler, ResponseHandler):
> 
> In general we have a bias against multiple inheritance.  I know here you're
> just inheriting from an ABC.  Since there is only one concrete implementation
> I don't think the interface-class really deserves to live.  (I know other
> projects do like them more than we do.)  I would probably just move the
> docstrings to the implementations.

There are three implemenations, actually.  The other two are in
bzrlib.smart.protocol: SmartClientRequestProtocolOne and
SmartClientRequestProtocolTwo.

It's mainly for those two classes benefit that I have the ABC; there was a lot
of different things tangled up in SmartClientRequestProtocolOne/Two, and this
ABC is part of what I used to keep it clear what the different parts are.

If we didn't have that messy legacy code, I'd agree with just collapsing it.

> +    def bytes_part_received(self, bytes):
> +        self._body_started = True
> +        self._bytes_parts.append(bytes)
> 
> should check here we already have the args?

No, it's valid for a response to receive multiple bytes parts (if it is a
streaming body).  See
<http://doc.bazaar-vcs.org/bzr.dev/developers/network-protocol.html#conventional-requests-and-responses>.

> +def _translate_error(error_tuple):
> +    # Many exceptions need some state from the requestor to be properly
> +    # translated (e.g. they need a branch object).  So this only translates a
> +    # few errors, and the rest are turned into a generic ErrorFromSmartServer.
> 
> Ah, I see what you mean.
> 
> One option would be to raise them without real contents and let the Remote*
> object catch them and poke in the right value.  So then if it doesn't
> explicitly handle them, at least we'll get something approximately right...

Yeah.  Ideally all RemoteBranch methods (for instance) would automagically fill
in a 'branch' attribute of any error raised within them.  But half-initialised
exceptions are likely to cause problems like being unrenderable, and also
sometimes an exception is actually about a different branch than “self” (e.g.
when fetching from another branch, the exception might be from either side...).

I'm sure we can do something better here, but it's not immediately obvious
exactly what :)

[...]
> @@ -76,7 +116,7 @@
>      def __init__(self, backing_transport, write_func, root_client_path='/'):
>          self._backing_transport = backing_transport
>          self._root_client_path = root_client_path
> -        self.excess_buffer = ''
> +        self.unused_data = ''
>          self._finished = False
>          self.in_buffer = ''
>          self.has_dispatched = False
> 
> Those should probably be private?

To bzrlib?  Ok, I'll _underscore them.

> 
> @@ -616,6 +705,8 @@
>              return 1
>          elif resp == ('ok', '2'):
>              return 2
> +        elif resp == ('ok', '3'):
> +            return 3
>          else:
>              raise errors.SmartProtocolError("bad response %r" % (resp,))
> 
> This also needs to be reverted I think.

Done.

-Andrew.