[MERGE] Return (and expect) '3' rather than '2' from the 'hello' verb (protocol v3 patch 4/7)
John Arbash Meinel
john at arbash-meinel.com
Wed May 7 20:01:12 BST 2008
-----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:
|> + def args_received(self, args):
|> + cmd = args[0]
|> + args = args[1:]
|> + try:
|> + command = self._commands.get(cmd)
|> + except LookupError:
|> + raise errors.SmartProtocolError("bad request %r" % (cmd,))
|> + self._command = command(self._backing_transport)
|> + self._run_handler_code(self._command.execute, args, {})
|> +
|>
|> ^- Shouldn't this also raise UnknownSmartMethod?
|
| Huh, yes. Thanks! I'll fix that.
|
|> So... won't older clients refuse to connect to a server which returns
|> '3' for OK? It seems like all the logic in the server is still present
|> to be able to talk in V2 mode. Shouldn't there be a way for clients to
|> request this?
|
| Sadly, yes. This was the least well-cooked part of the patch set.
|
| I've attached a merge directive (relative to the rest of the smart protocol v3
| work) that fixes this.
|
| It implements detection the server protocol version a better way: the first time
| a request is made, it simply tries v3, then it tries v2, then it gives up.
| After that, it remembers the version it detected. 'hello' is no longer used by
| this client.
|
My concern with this is the problem we had with the "call_with_body_bytes" for
unknown protocols. Where calling the server with an unknown method made it start
doing bad things. We know disconnect and reconnect, but that is still somewhat
unsafe. Considering the server still things you made multiple requests, and is
still failing, we are just hiding that from the client.
I guess V3 is a much bigger change from V2, or even the V2 extra-body stuff. So
if V3 fails, it will fail right away, in a predictable manner for any call that
we might make. If it doesn't, then I think we need to reconsider how we want to
do it.
| An alternative solution I considered and rejected was adding a new 'helloV3'
| verb (maybe called 'SupportedVersions') that could return a list of supported
| protocol versions, which would be more future proof than the existing 'hello'
| verb. This would have involved some moderately complicated code to fallback to
| 'hello' when 'helloV3' isn't there, plus adding yet another verb. And we'd need
| to be pretty careful about how we defined the verb to make sure it was *really*
| future-proof, unlike our last effort. And really we've wanted to stop needing
| 'hello' for a long time; it's just a wasted round-trip when the client's default
| protocol version is understood by the server. So even though I started writing
| this solution, after chatting with Martin I decided it wasn't worth it.
|
I would *like* to get rid of hello. I'm just worried that we are sending
"random" bytes to a server and expecting it to respond in a deterministic way
for any call that we are trying to send it. Because V3 always has a clear v3
header, it might be enough.
...
| I'm aiming to get the v3 code landed in time for bzr 1.5 rc1. I believe this
| patch resolves the largest outstanding issue, so a review of this patch would be
| an enormous help.
|
| -Andrew.
~ self._base = base
- -
- - def _build_client_protocol(self):
- - version = self._medium.protocol_version()
+ self._protocol_version = None
+ if headers is None:
+ self._headers = {'Software version': bzrlib.__version__}
+ else:
+ self._headers = dict(headers)
+
^- I think this should probably be called "self._default_headers" to make it
clear that this is the suggested headers to be sent, and not the absolute ones.
+ def _call(self, encoder, method, args, body=None, readv_body=None):
+ encoder.set_headers(self._headers)
+ if body is not None:
+ encoder.call_with_body_bytes((method, ) + args, body)
+ elif readv_body is not None:
+ encoder.call_with_body_readv_array((method, ) + args,
+ readv_body)
+ else:
+ encoder.call(method, *args)
^- A check that 'body is not None and readv_body is not None' would be nice.
Just to help developers that accidentally set both.
It also seems like something should be "returned" here, but I guess that is done
in the "read_response" portion.
+ def _call_and_read_response(self, method, args, body=None, readv_body=None,
+ expect_response_body=True):
+ if self._protocol_version is not None:
+ encoder, response_handler = self._construct_protocol(
+ self._protocol_version)
+ self._call(encoder, method, args, body=body, readv_body=readv_body)
+ return (response_handler.read_response_tuple(
+ expect_body=expect_response_body),
+ response_handler)
+ else:
+ for protocol_version in [3, 2]:
+ encoder, response_handler = self._construct_protocol(
+ protocol_version)
+ self._call(encoder, method, args, body=body,
+ readv_body=readv_body)
+ 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()
+ continue
+ else:
+ self._protocol_version = protocol_version
+ return response_tuple, response_handler
+ raise errors.SmartProtocolError(
+ 'Server is not a Bazaar server: ' + str(err))
+
^- There seems to be a lot of duplicated code between the "_protocol_version is
not None" if statement, and the following loop. Could you abstract this into a
"_call_and_read_response(protocol_version = XXX) instead?
I know it might mean trapping UnexpectedProtocolVersionMarker all the time, but
you could do something like:
except errors.UnexpectedProtocolVersionMarker:
~ if self._protocol_version is not None:
~ # This server already claimed to support this version
~ raise
IMO it would be cleaner.
=== modified file 'bzrlib/smart/protocol.py'
- --- bzrlib/smart/protocol.py 2008-04-23 22:43:14 +0000
+++ bzrlib/smart/protocol.py 2008-05-07 14:06:29 +0000
@@ -89,6 +89,9 @@
~ """
~ raise NotImplementedError(self.call_with_body_readv_array)
+ def set_headers(self, headers):
+ raise NotImplementedError(self.set_headers)
+
^- Following the above, this might be "set_default_headers". Unless it really is
the full set of headers.
Non-default headers seem like they belong on the call_with...() itself, though,
rather than being set on the Protocol object.
- - self._number_needed_bytes = 4
- - self.state_accept = self._state_accept_expecting_headers
+ if expect_version_marker:
+ self.state_accept = self._state_accept_expecting_protocol_version
+ # We're expecting at least the protocol version marker + some
+ # headers.
+ self._number_needed_bytes = len(MESSAGE_VERSION_THREE) + 4
+ else:
+ self.state_accept = self._state_accept_expecting_headers
+ self._number_needed_bytes = 4
^- This seems like it should have some amount of knowledge whether
MESSAGE_VERSION_THREE is actually the returned message. Or maybe that just needs
to happen when MESSAGE_VERSION_FOUR comes out.
+ needed_bytes = len(MESSAGE_VERSION_THREE) - len(self._in_buffer)
+ if needed_bytes > 0:
+ if not MESSAGE_VERSION_THREE.startswith(self._in_buffer):
+ # We have enough bytes to know the protocol version is wrong
+ raise errors.UnexpectedProtocolVersionMarker(self._in_buffer)
+ raise _NeedMoreBytes(len(MESSAGE_VERSION_THREE))
^- This was a bit confusing when I first read it. certainly the startswith() is
correct, even if it looks the wrong way around. It seems correct, but I spent 2
minutes trying to rewrite it to be "correct" before I realized it already was.
Maybe it is just me, or maybe it is confusing.
~ def _write_structure(self, args):
@@ -969,6 +993,7 @@
~ def __init__(self, write_func):
~ _ProtocolThreeEncoder.__init__(self, write_func)
~ self.response_sent = False
+ self._headers = {'Software version': bzrlib.__version__}
^- Again, _default_headers ?
+ def set_headers(self, headers):
+ self._headers = dict(headers)
+
^- If we require headers to be a dict, I feel like "headers.copy()" is clearer
about what you are doing. (Creating a copy so that mutations don't change your
internal header list.)
If you are trying to support .set_headers([('foo', 'bar'), ('baz', 'bing')])
then what you did is fine.
I'm a little curious why you need to be setting "expect_version_marker=True"
everywhere. Isn't this always True for V3 requests? Is it ever true/not true for
V2 requests? Otherwise it seems like it could be set by the protocol, rather
than having to be passed around.
Specifically, if it is a V3 response decoder, then shouldn't it always
expect_version_marker=True?
MockMedium seems fine. I sort of wondered if we could get away from bytes on the
wire, but I realized that we really do (at least at certain points) want to test
bytes on the wire for these things.
I don't think it is necessarily the best way to test falling back to V2 from a
V3 server, but it is ok.
Certainly I like to see bytes-on-the-wire tests because they help ensure
compatibility across releases.
I did notice this:
+ medium.expect_request(
+ 'bzr message 3 (bzr 1.3)\n\x00\x00\x00\x02de' +
+ 's\x00\x00\x00\x1el11:method-name5:arg 15:arg 2ee',
+ 'bzr response 2\nfailed\n\n')
^- And I would expect that to be at least (bzr 1.5) for it to be merged. But
that is probably a different patch.
It just has to happen first, so that we don't end up with a (bzr 1.3) in a
bzr.dev release that we have to worry about.
BB:tweak
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkgh/HgACgkQJdeBCYSNAAP2mACfQuZX3+Fkcg+D9g7e60tq07x1
69wAoMmJLg7QJ3zrubYYIeo4XFXNftoc
=pYNV
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list