No subject


Thu Oct 18 14:49:24 BST 2007


for ProtocolTwo like I expected. :-( Manual testing was good though. See
below.

> This situation does not necessarily mean that this is during “start up” (which
> isn't a defined term here) or even that this is due to the connection failing.
> This could happen if the first request on pipe/socket succeeds, but then the
> connection is closed before the second one is answered.
> 
>> +                else:
>> +                    # disconnected after some data received
>> +                    raise errors.ConnectionReset("connection timeout")
> 
> I'd simply call both of these cases “Connection closed”, or perhaps
> “Connection closed unexpectedly”.  If the user wants to know more about exactly
> at which point the conversation is being broken, perhaps they should be using
> -Dhpss.

Good point.

> I think if we want to distinguish between “could not establish connection” and
> “connection closed in general” then we should do it at a slightly different
> layer.  (I think the issue here is when running SSH in a subprocess we fail to
> notice connection errors until we try to read bytes from the subprocess pipe,
> whereas with the Paramiko implementation it will raise a more useful exception
> at connect time, well before read_bytes is involved.  Probably this isn't worth
> the error to fix, as the SSH subprocess should have delivered relevant errors to
> the user on stderr already.)

Agreed.

>> === modified file 'bzrlib/tests/test_smart_transport.py'
>> --- bzrlib/tests/test_smart_transport.py	2007-10-12 05:26:46 +0000
>> +++ bzrlib/tests/test_smart_transport.py	2007-10-24 04:26:21 +0000
>> @@ -1424,6 +1424,26 @@
>>          self.assertOffsetSerialisation([(1,2), (3,4), (100, 200)],
>>              '1,2\n3,4\n100,200', self.client_protocol)
>>  
>> +    def test_connection_error_on_startup(self):
> [...]
>> +    def test_connection_error_during_line(self):
> [...]
> 
> As I mention above, these tests need to be added to TestSmartProtocolTwo as
> well.  You can probably do this best by using self.client_protocol_class rather
> than protocol.SmartClientRequestProtocolOne, and moving these into
> CommonSmartProtocolTestMixin.
> 
> These tests are good, but they highlight another deficiency: there's no tests I
> can see for a connection being closed during read_body_bytes.  If it's not too
> hard, it would be great to fix that while you're here (you'll probably find the
> build_protocol_waiting_for_body helper method useful for writing that test).
> 
> I think what you've fixed is more than worthwhile on its own, so if you'd prefer
> I'd be fine with filing a bug about testing (and possibly improving) the
> behaviour if a connection drops out during read_body_bytes.  Errors during
> read_response_tuple are much more common I expect (because that's where DNS
> errors, authentication failures, etc will make themselves known).

I think it's worth landing the improvement while recognising it needs to
be better. So I've simplified it to the same exception regardless of
when the connection is closed, tested it and merged it. When we catch up
in person next week, let's find some time so you can explain to me how
to write the tests - and get them passing :-) - correctly.

Ian C.



More information about the bazaar mailing list