Bazaar

Merge lp:~jameinel/bzr/2.1-client-read-reconnect-819604 into lp:bzr/2.1

  1. 2.1-client-read-reconnect-819604
  2. Merge into 2.1
Proposed by John A Meinel
Status: Work in progress
Proposed branch: lp:~jameinel/bzr/2.1-client-read-reconnect-819604
Merge into: lp:bzr/2.1
Prerequisite: lp:~jameinel/bzr/2.1-categorize-requests-819604
Diff against target: 138 lines (+79/-5)
3 files modified
NEWS (+5/-0)
bzrlib/smart/client.py (+36/-2)
bzrlib/tests/test_smart_transport.py (+38/-3)
To merge this branch: bzr merge lp:~jameinel/bzr/2.1-client-read-reconnect-819604
Reviewer Review Type Date Requested Status
bzr-core Pending
Review via email: mp+78839@code.launchpad.net

Commit message

Bug #819604, retry the request if we get ConnectionReset while reading as well.

Description of the change

This is the final step in the chain.

This adds the ability to retry the request if we get a ConnectionReset while reading the response from the server.

Note that this only retries the request while parsing the response header (ok, error, etc). Not while reading the actual body content. However, as mentioned in the beginning, that is the use case that we really care about. We are trying to allow the server to soft-disconnect us, in which case it shouldn't send any response at all.

(The other thing we could have done is upgraded the RPC to allow the server to send a 'reconnect' message, but that wouldn't have been compatible with older clients, especially since they wouldn't have been listening for that response at the end of a previous message, etc.)

Note that I've tested this by running bzr.dev on devpad, and using 'bzr branch bzr+ssh://...' and then while on devpad, running 'kill -SIGHUP PID' on whatever bzr is currently active. I wasn't able to actually stop the branch from finishing, even though I killed it in a few different places.

And the client appropriately reported that it got ConnectionReset but that it was going to try again.

To post a comment you must log in.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2011-08-19 12:49:28 +0000
3+++ NEWS 2011-10-10 13:23:11 +0000
4@@ -43,6 +43,11 @@
5
6 (John Arbash Meinel, #609187, #812928)
7
8+* Teach the bzr client how to reconnect if we get ``ConnectionReset``
9+ while making an RPC request. This doesn't handle all possible network
10+ disconnects, but it should at least handle when the server is asked to
11+ shutdown gracefully. (John Arbash Meinel, #819604)
12+
13
14 Improvements
15 ************
16
17=== modified file 'bzrlib/smart/client.py'
18--- bzrlib/smart/client.py 2011-10-10 13:23:11 +0000
19+++ bzrlib/smart/client.py 2011-10-10 13:23:11 +0000
20@@ -14,6 +14,11 @@
21 # along with this program; if not, write to the Free Software
22 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
23
24+from bzrlib import lazy_import
25+lazy_import.lazy_import(globals(), """
26+from bzrlib.smart import request as _mod_request
27+""")
28+
29 import bzrlib
30 from bzrlib.smart import message, protocol
31 from bzrlib import (
32@@ -149,6 +154,22 @@
33 else:
34 return self._call(protocol_version)
35
36+ def _is_safe_to_send_twice(self):
37+ """Check if the current method is re-entrant safe."""
38+ if self.body_stream is not None or 'noretry' in debug.debug_flags:
39+ # We can't restart a body stream that has already been consumed.
40+ return False
41+ request_type = _mod_request.request_handlers.get_info(self.method)
42+ if request_type in ('read', 'idem', 'semi'):
43+ return True
44+ # If we have gotten this far, 'stream' cannot be retried, because we
45+ # already consumed the local stream.
46+ if request_type in ('semivfs', 'mutate', 'stream'):
47+ return False
48+ trace.mutter('Unknown request type: %s for method %s'
49+ % (request_type, self.method))
50+ return False
51+
52 def _run_call_hooks(self):
53 if not _SmartClient.hooks['call']:
54 return
55@@ -164,8 +185,21 @@
56 where the code will be to retry requests if the connection is closed.
57 """
58 response_handler = self._send(protocol_version)
59- response_tuple = response_handler.read_response_tuple(
60- expect_body=self.expect_response_body)
61+ try:
62+ response_tuple = response_handler.read_response_tuple(
63+ expect_body=self.expect_response_body)
64+ except errors.ConnectionReset, e:
65+ self.client._medium.reset()
66+ if not self._is_safe_to_send_twice():
67+ raise
68+ trace.warning('ConnectionReset reading response for %r, retrying'
69+ % (self.method,))
70+ trace.log_exception_quietly()
71+ encoder, response_handler = self._construct_protocol(
72+ protocol_version)
73+ self._send_no_retry(encoder)
74+ response_tuple = response_handler.read_response_tuple(
75+ expect_body=self.expect_response_body)
76 return (response_tuple, response_handler)
77
78 def _call_determining_protocol_version(self):
79
80=== modified file 'bzrlib/tests/test_smart_transport.py'
81--- bzrlib/tests/test_smart_transport.py 2011-10-10 13:23:11 +0000
82+++ bzrlib/tests/test_smart_transport.py 2011-10-10 13:23:11 +0000
83@@ -3382,10 +3382,10 @@
84
85 class Test_SmartClientRequest(tests.TestCase):
86
87- def make_client_with_failing_medium(self, fail_at_write=True):
88- response = StringIO()
89+ def make_client_with_failing_medium(self, fail_at_write=True, response=''):
90+ response_io = StringIO(response)
91 output = StringIO()
92- vendor = FirstRejectedStringIOSSHVendor(response, output,
93+ vendor = FirstRejectedStringIOSSHVendor(response_io, output,
94 fail_at_write=fail_at_write)
95 client_medium = medium.SmartSSHClientMedium(
96 'a host', 'a port', 'a user', 'a pass', 'base', vendor,
97@@ -3393,6 +3393,41 @@
98 smart_client = client._SmartClient(client_medium, headers={})
99 return output, vendor, smart_client
100
101+ def make_response(self, args, body=None, body_stream=None):
102+ response_io = StringIO()
103+ response = _mod_request.SuccessfulSmartServerResponse(args, body=body,
104+ body_stream=body_stream)
105+ responder = protocol.ProtocolThreeResponder(response_io.write)
106+ responder.send_response(response)
107+ return response_io.getvalue()
108+
109+ def test__call_doesnt_retry_append(self):
110+ response = self.make_response(('appended', '8'))
111+ output, vendor, smart_client = self.make_client_with_failing_medium(
112+ fail_at_write=False, response=response)
113+ smart_request = client._SmartClientRequest(smart_client, 'append',
114+ ('foo', ''), body='content\n')
115+ self.assertRaises(errors.ConnectionReset, smart_request._call, 3)
116+
117+ def test__call_retries_get_bytes(self):
118+ response = self.make_response(('ok',), 'content\n')
119+ output, vendor, smart_client = self.make_client_with_failing_medium(
120+ fail_at_write=False, response=response)
121+ smart_request = client._SmartClientRequest(smart_client, 'get',
122+ ('foo',))
123+ response, response_handler = smart_request._call(3)
124+ self.assertEqual(('ok',), response)
125+ self.assertEqual('content\n', response_handler.read_body_bytes())
126+
127+ def test__call_noretry_get_bytes(self):
128+ debug.debug_flags.add('noretry')
129+ response = self.make_response(('ok',), 'content\n')
130+ output, vendor, smart_client = self.make_client_with_failing_medium(
131+ fail_at_write=False, response=response)
132+ smart_request = client._SmartClientRequest(smart_client, 'get',
133+ ('foo',))
134+ self.assertRaises(errors.ConnectionReset, smart_request._call, 3)
135+
136 def test__send_no_retry_pipes(self):
137 client_read, server_write = create_file_pipes()
138 server_read, client_write = create_file_pipes()

Subscribers

People subscribed via source and target branches