Merge lp:~spiv/bzr/no_until_no_eintr into lp:bzr
- no_until_no_eintr
- Merge into bzr.dev
Status: | Merged |
---|---|
Approved by: | Martin Pool |
Approved revision: | no longer in the source branch. |
Merged at revision: | not available |
Proposed branch: | lp:~spiv/bzr/no_until_no_eintr |
Merge into: | lp:bzr |
Diff against target: |
424 lines (+126/-66) 5 files modified
NEWS (+10/-3) bzrlib/osutils.py (+77/-18) bzrlib/smart/medium.py (+30/-44) bzrlib/smart/protocol.py (+7/-1) bzrlib/smart/server.py (+2/-0) |
To merge this branch: | bzr merge lp:~spiv/bzr/no_until_no_eintr |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Packman (community) | Needs Information | ||
Martin Pool | Approve | ||
Review via email: mp+21699@code.launchpad.net |
Commit message
(andrew) Either correctly handle EINTR or don't handle it at all. (#496813)
Description of the change
This change removes the unsafe uses of until_no_eintr, based on the bug reports and patches from the very patient Martin <gz>. Unlike Martin <gz>'s original patch it doesn't remove the until_no_eintr helper entirely, because it is safe (and useful) to use for reading from a pipe.
(In other changes we've already reduced the number of signal handlers we install that will trigger EINTR, but it can still occur, so, when practical, we may as well handle it correctly rather than fail.)
The most alarming diff hunk is:
- return '\x01'.join(args) + '\n'
+ joined = '\x01'.join(args) + '\n'
+ if type(joined) is unicode:
+ # XXX: We should fix things so this never happens! -AJB, 04032010.
+ mutter('response args contain unicode, should be only bytes: %r',
+ joined)
+ joined = joined.
+ return joined
This is a latent bug in the HPSS protocol v1 (and maybe v2) code now exposed by the change to use buffer() in send_all. This change explicitly does the convert-to-ASCII that was being done anyway. v3 is unaffected.
The diff applies cleanly to 2.1 (aside from the NEWS entry, of course), and I'll submit a backport for review once this proposal is accepted.
Martin Pool (mbp) wrote : | # |
If Martin <gz> gets the chance to review it that would be good, otherwise I suggest you merge.
Martin Packman (gz) wrote : | # |
Interesting on the encode tuple unicode issue, I didn't see any issues there in testing. Nit, you've got the ISO 8601 basic format wrong, it's big endian.
Having thought about this a bit more, I'm not mad about a bzrlib.smart.medium focused change. The module is already wrappers around wrappers around wrappers, and it's too hard to work out what's going on. Case in point - as far as I can tell, SmartSSHClientM
I've been waiting for magic bullet inspiration on a clean fix here, but nothing's struck yet. Perhaps a different approach, like unifying on one lowest-
Andrew Bennetts (spiv) wrote : | # |
Martin [gz] wrote:
[...]
> I've been waiting for magic bullet inspiration on a clean fix here, but
> nothing's struck yet. Perhaps a different approach, like unifying on one
> lowest-
> those throughout would be clearer and mean less copy-code to get subtly wrong
> in certain versions.
I've been wondering about that approach too. It might simplify transport
activity reporting too.
-Andrew.
Andrew Bennetts (spiv) wrote : | # |
Martin [gz] wrote:
> Interesting on the encode tuple unicode issue, I didn't see any issues there
> in testing. Nit, you've got the ISO 8601 basic format wrong, it's big endian.
Heh, fixed :)
> Having thought about this a bit more, I'm not mad about a bzrlib.smart.medium
> focused change. The module is already wrappers around wrappers around
> wrappers, and it's too hard to work out what's going on. Case in point - as
> far as I can tell, SmartSSHClientM
> uses an unsafe wrapper for sockets pre-2.7 in _ParamikoSSHCon
> non-EINTR-protected pipes in SSHSubprocess. This means we're not covering
> codepaths like the one bug 341535 highlighted.
That's true. However I've just sent this patch to PQM anyway because I think
it's still a clear improvement. Here's my reasoning:
- I think regressing 341535 is better than risking silent corruption of HPSS data
(and certainly better than a confusing error caused by corrupted HPSS data),
- Our SIGWINCH handler uses siginterrupt to avoid triggering EINTR, and
- bzrlib's only other signal handler is SIGQUIT, which is a developer tool not
a production feature.
I'll reopen 341535, of course.
Thanks for another thoughtful review!
Preview Diff
1 | === modified file 'NEWS' | |||
2 | --- NEWS 2010-03-25 12:33:15 +0000 | |||
3 | +++ NEWS 2010-03-26 04:50:44 +0000 | |||
4 | @@ -86,6 +86,9 @@ | |||
5 | 86 | * Added docstring for ``Tree.iter_changes`` | 86 | * Added docstring for ``Tree.iter_changes`` |
6 | 87 | (John Arbash Meinel, #304182) | 87 | (John Arbash Meinel, #304182) |
7 | 88 | 88 | ||
8 | 89 | * Allow additional arguments to | ||
9 | 90 | ``RemoteRepository.add_inventory_by_delta()``. (Jelmer Vernooij, #532631) | ||
10 | 91 | |||
11 | 89 | * Allow exporting a single file using ``bzr export``. | 92 | * Allow exporting a single file using ``bzr export``. |
12 | 90 | (Michal Junák, #511987) | 93 | (Michal Junák, #511987) |
13 | 91 | 94 | ||
14 | @@ -144,6 +147,13 @@ | |||
15 | 144 | * Fix stub sftp test server to call os.getcwdu(). | 147 | * Fix stub sftp test server to call os.getcwdu(). |
16 | 145 | (Vincent Ladeuil, #526211, #526353) | 148 | (Vincent Ladeuil, #526211, #526353) |
17 | 146 | 149 | ||
18 | 150 | * Many IO operations that returned ``EINTR`` were retried even if it | ||
19 | 151 | wasn't safe to do so via careless use of ``until_no_eintr``. Bazaar now | ||
20 | 152 | only retries operations that are safe to retry, and in some cases has | ||
21 | 153 | switched to operations that can be retried (e.g. ``sock.send`` rather than | ||
22 | 154 | ``sock.sendall``). | ||
23 | 155 | (Andrew Bennetts, Martin <gzlist@googlemail.com>, #496813) | ||
24 | 156 | |||
25 | 147 | * Path conflicts now support --take-this and --take-other even when a | 157 | * Path conflicts now support --take-this and --take-other even when a |
26 | 148 | deletion is involved. | 158 | deletion is involved. |
27 | 149 | (Vincent Ladeuil, #531967) | 159 | (Vincent Ladeuil, #531967) |
28 | @@ -163,9 +173,6 @@ | |||
29 | 163 | the debugger won't kill the session. | 173 | the debugger won't kill the session. |
30 | 164 | (Martin <gzlist@googlemail.com>, #162502) | 174 | (Martin <gzlist@googlemail.com>, #162502) |
31 | 165 | 175 | ||
32 | 166 | * Allow additional arguments to | ||
33 | 167 | ``RemoteRepository.add_inventory_by_delta()``. (Jelmer Vernooij, #532631) | ||
34 | 168 | |||
35 | 169 | * Tolerate patches with leading noise in ``bzr-handle-patch``. | 176 | * Tolerate patches with leading noise in ``bzr-handle-patch``. |
36 | 170 | (Toshio Kuratomi, Martin Pool, #502076) | 177 | (Toshio Kuratomi, Martin Pool, #502076) |
37 | 171 | 178 | ||
38 | 172 | 179 | ||
39 | === modified file 'bzrlib/osutils.py' | |||
40 | --- bzrlib/osutils.py 2010-03-25 17:04:08 +0000 | |||
41 | +++ bzrlib/osutils.py 2010-03-26 04:50:44 +0000 | |||
42 | @@ -40,6 +40,7 @@ | |||
43 | 40 | rmtree, | 40 | rmtree, |
44 | 41 | ) | 41 | ) |
45 | 42 | import signal | 42 | import signal |
46 | 43 | import socket | ||
47 | 43 | import subprocess | 44 | import subprocess |
48 | 44 | import tempfile | 45 | import tempfile |
49 | 45 | from tempfile import ( | 46 | from tempfile import ( |
50 | @@ -50,12 +51,16 @@ | |||
51 | 50 | from bzrlib import ( | 51 | from bzrlib import ( |
52 | 51 | cache_utf8, | 52 | cache_utf8, |
53 | 52 | errors, | 53 | errors, |
54 | 54 | trace, | ||
55 | 53 | win32utils, | 55 | win32utils, |
56 | 54 | trace, | ||
57 | 55 | ) | 56 | ) |
58 | 56 | |||
59 | 57 | """) | 57 | """) |
60 | 58 | 58 | ||
61 | 59 | from bzrlib.symbol_versioning import ( | ||
62 | 60 | deprecated_function, | ||
63 | 61 | deprecated_in, | ||
64 | 62 | ) | ||
65 | 63 | |||
66 | 59 | # sha and md5 modules are deprecated in python2.6 but hashlib is available as | 64 | # sha and md5 modules are deprecated in python2.6 but hashlib is available as |
67 | 60 | # of 2.5 | 65 | # of 2.5 |
68 | 61 | if sys.version_info < (2, 5): | 66 | if sys.version_info < (2, 5): |
69 | @@ -1957,40 +1962,82 @@ | |||
70 | 1957 | return socket.gethostname().decode(get_user_encoding()) | 1962 | return socket.gethostname().decode(get_user_encoding()) |
71 | 1958 | 1963 | ||
72 | 1959 | 1964 | ||
74 | 1960 | def recv_all(socket, bytes): | 1965 | # We must not read/write any more than 64k at a time from/to a socket so we |
75 | 1966 | # don't risk "no buffer space available" errors on some platforms. Windows in | ||
76 | 1967 | # particular is likely to throw WSAECONNABORTED or WSAENOBUFS if given too much | ||
77 | 1968 | # data at once. | ||
78 | 1969 | MAX_SOCKET_CHUNK = 64 * 1024 | ||
79 | 1970 | |||
80 | 1971 | def read_bytes_from_socket(sock, report_activity=None, | ||
81 | 1972 | max_read_size=MAX_SOCKET_CHUNK): | ||
82 | 1973 | """Read up to max_read_size of bytes from sock and notify of progress. | ||
83 | 1974 | |||
84 | 1975 | Translates "Connection reset by peer" into file-like EOF (return an | ||
85 | 1976 | empty string rather than raise an error), and repeats the recv if | ||
86 | 1977 | interrupted by a signal. | ||
87 | 1978 | """ | ||
88 | 1979 | while 1: | ||
89 | 1980 | try: | ||
90 | 1981 | bytes = sock.recv(max_read_size) | ||
91 | 1982 | except socket.error, e: | ||
92 | 1983 | eno = e.args[0] | ||
93 | 1984 | if eno == getattr(errno, "WSAECONNRESET", errno.ECONNRESET): | ||
94 | 1985 | # The connection was closed by the other side. Callers expect | ||
95 | 1986 | # an empty string to signal end-of-stream. | ||
96 | 1987 | return "" | ||
97 | 1988 | elif eno == errno.EINTR: | ||
98 | 1989 | # Retry the interrupted recv. | ||
99 | 1990 | continue | ||
100 | 1991 | raise | ||
101 | 1992 | else: | ||
102 | 1993 | if report_activity is not None: | ||
103 | 1994 | report_activity(len(bytes), 'read') | ||
104 | 1995 | return bytes | ||
105 | 1996 | |||
106 | 1997 | |||
107 | 1998 | def recv_all(socket, count): | ||
108 | 1961 | """Receive an exact number of bytes. | 1999 | """Receive an exact number of bytes. |
109 | 1962 | 2000 | ||
110 | 1963 | Regular Socket.recv() may return less than the requested number of bytes, | 2001 | Regular Socket.recv() may return less than the requested number of bytes, |
112 | 1964 | dependning on what's in the OS buffer. MSG_WAITALL is not available | 2002 | depending on what's in the OS buffer. MSG_WAITALL is not available |
113 | 1965 | on all platforms, but this should work everywhere. This will return | 2003 | on all platforms, but this should work everywhere. This will return |
114 | 1966 | less than the requested amount if the remote end closes. | 2004 | less than the requested amount if the remote end closes. |
115 | 1967 | 2005 | ||
116 | 1968 | This isn't optimized and is intended mostly for use in testing. | 2006 | This isn't optimized and is intended mostly for use in testing. |
117 | 1969 | """ | 2007 | """ |
118 | 1970 | b = '' | 2008 | b = '' |
121 | 1971 | while len(b) < bytes: | 2009 | while len(b) < count: |
122 | 1972 | new = until_no_eintr(socket.recv, bytes - len(b)) | 2010 | new = read_bytes_from_socket(socket, None, count - len(b)) |
123 | 1973 | if new == '': | 2011 | if new == '': |
124 | 1974 | break # eof | 2012 | break # eof |
125 | 1975 | b += new | 2013 | b += new |
126 | 1976 | return b | 2014 | return b |
127 | 1977 | 2015 | ||
128 | 1978 | 2016 | ||
130 | 1979 | def send_all(socket, bytes, report_activity=None): | 2017 | def send_all(sock, bytes, report_activity=None): |
131 | 1980 | """Send all bytes on a socket. | 2018 | """Send all bytes on a socket. |
136 | 1981 | 2019 | ||
137 | 1982 | Regular socket.sendall() can give socket error 10053 on Windows. This | 2020 | Breaks large blocks in smaller chunks to avoid buffering limitations on |
138 | 1983 | implementation sends no more than 64k at a time, which avoids this problem. | 2021 | some platforms, and catches EINTR which may be thrown if the send is |
139 | 1984 | 2022 | interrupted by a signal. | |
140 | 2023 | |||
141 | 2024 | This is preferred to socket.sendall(), because it avoids portability bugs | ||
142 | 2025 | and provides activity reporting. | ||
143 | 2026 | |||
144 | 1985 | :param report_activity: Call this as bytes are read, see | 2027 | :param report_activity: Call this as bytes are read, see |
145 | 1986 | Transport._report_activity | 2028 | Transport._report_activity |
146 | 1987 | """ | 2029 | """ |
153 | 1988 | chunk_size = 2**16 | 2030 | sent_total = 0 |
154 | 1989 | for pos in xrange(0, len(bytes), chunk_size): | 2031 | byte_count = len(bytes) |
155 | 1990 | block = bytes[pos:pos+chunk_size] | 2032 | while sent_total < byte_count: |
156 | 1991 | if report_activity is not None: | 2033 | try: |
157 | 1992 | report_activity(len(block), 'write') | 2034 | sent = sock.send(buffer(bytes, sent_total, MAX_SOCKET_CHUNK)) |
158 | 1993 | until_no_eintr(socket.sendall, block) | 2035 | except socket.error, e: |
159 | 2036 | if e.args[0] != errno.EINTR: | ||
160 | 2037 | raise | ||
161 | 2038 | else: | ||
162 | 2039 | sent_total += sent | ||
163 | 2040 | report_activity(sent, 'write') | ||
164 | 1994 | 2041 | ||
165 | 1995 | 2042 | ||
166 | 1996 | def dereference_path(path): | 2043 | def dereference_path(path): |
167 | @@ -2067,7 +2114,18 @@ | |||
168 | 2067 | 2114 | ||
169 | 2068 | 2115 | ||
170 | 2069 | def until_no_eintr(f, *a, **kw): | 2116 | def until_no_eintr(f, *a, **kw): |
172 | 2070 | """Run f(*a, **kw), retrying if an EINTR error occurs.""" | 2117 | """Run f(*a, **kw), retrying if an EINTR error occurs. |
173 | 2118 | |||
174 | 2119 | WARNING: you must be certain that it is safe to retry the call repeatedly | ||
175 | 2120 | if EINTR does occur. This is typically only true for low-level operations | ||
176 | 2121 | like os.read. If in any doubt, don't use this. | ||
177 | 2122 | |||
178 | 2123 | Keep in mind that this is not a complete solution to EINTR. There is | ||
179 | 2124 | probably code in the Python standard library and other dependencies that | ||
180 | 2125 | may encounter EINTR if a signal arrives (and there is signal handler for | ||
181 | 2126 | that signal). So this function can reduce the impact for IO that bzrlib | ||
182 | 2127 | directly controls, but it is not a complete solution. | ||
183 | 2128 | """ | ||
184 | 2071 | # Borrowed from Twisted's twisted.python.util.untilConcludes function. | 2129 | # Borrowed from Twisted's twisted.python.util.untilConcludes function. |
185 | 2072 | while True: | 2130 | while True: |
186 | 2073 | try: | 2131 | try: |
187 | @@ -2077,6 +2135,7 @@ | |||
188 | 2077 | continue | 2135 | continue |
189 | 2078 | raise | 2136 | raise |
190 | 2079 | 2137 | ||
191 | 2138 | |||
192 | 2080 | def re_compile_checked(re_string, flags=0, where=""): | 2139 | def re_compile_checked(re_string, flags=0, where=""): |
193 | 2081 | """Return a compiled re, or raise a sensible error. | 2140 | """Return a compiled re, or raise a sensible error. |
194 | 2082 | 2141 | ||
195 | 2083 | 2142 | ||
196 | === modified file 'bzrlib/smart/medium.py' | |||
197 | --- bzrlib/smart/medium.py 2010-02-17 17:11:16 +0000 | |||
198 | +++ bzrlib/smart/medium.py 2010-03-26 04:50:44 +0000 | |||
199 | @@ -24,15 +24,14 @@ | |||
200 | 24 | bzrlib/transport/smart/__init__.py. | 24 | bzrlib/transport/smart/__init__.py. |
201 | 25 | """ | 25 | """ |
202 | 26 | 26 | ||
203 | 27 | import errno | ||
204 | 28 | import os | 27 | import os |
205 | 29 | import socket | ||
206 | 30 | import sys | 28 | import sys |
207 | 31 | import urllib | 29 | import urllib |
208 | 32 | 30 | ||
209 | 33 | from bzrlib.lazy_import import lazy_import | 31 | from bzrlib.lazy_import import lazy_import |
210 | 34 | lazy_import(globals(), """ | 32 | lazy_import(globals(), """ |
211 | 35 | import atexit | 33 | import atexit |
212 | 34 | import socket | ||
213 | 36 | import thread | 35 | import thread |
214 | 37 | import weakref | 36 | import weakref |
215 | 38 | 37 | ||
216 | @@ -47,14 +46,13 @@ | |||
217 | 47 | from bzrlib.smart import client, protocol, request, vfs | 46 | from bzrlib.smart import client, protocol, request, vfs |
218 | 48 | from bzrlib.transport import ssh | 47 | from bzrlib.transport import ssh |
219 | 49 | """) | 48 | """) |
220 | 50 | #usually already imported, and getting IllegalScoperReplacer on it here. | ||
221 | 51 | from bzrlib import osutils | 49 | from bzrlib import osutils |
222 | 52 | 50 | ||
228 | 53 | # We must not read any more than 64k at a time so we don't risk "no buffer | 51 | # Throughout this module buffer size parameters are either limited to be at |
229 | 54 | # space available" errors on some platforms. Windows in particular is likely | 52 | # most _MAX_READ_SIZE, or are ignored and _MAX_READ_SIZE is used instead. |
230 | 55 | # to give error 10053 or 10055 if we read more than 64k from a socket. | 53 | # For this module's purposes, MAX_SOCKET_CHUNK is a reasonable size for reads |
231 | 56 | _MAX_READ_SIZE = 64 * 1024 | 54 | # from non-sockets as well. |
232 | 57 | 55 | _MAX_READ_SIZE = osutils.MAX_SOCKET_CHUNK | |
233 | 58 | 56 | ||
234 | 59 | def _get_protocol_factory_for_bytes(bytes): | 57 | def _get_protocol_factory_for_bytes(bytes): |
235 | 60 | """Determine the right protocol factory for 'bytes'. | 58 | """Determine the right protocol factory for 'bytes'. |
236 | @@ -276,9 +274,9 @@ | |||
237 | 276 | def _serve_one_request_unguarded(self, protocol): | 274 | def _serve_one_request_unguarded(self, protocol): |
238 | 277 | while protocol.next_read_size(): | 275 | while protocol.next_read_size(): |
239 | 278 | # We can safely try to read large chunks. If there is less data | 276 | # We can safely try to read large chunks. If there is less data |
243 | 279 | # than _MAX_READ_SIZE ready, the socket wil just return a short | 277 | # than MAX_SOCKET_CHUNK ready, the socket will just return a |
244 | 280 | # read immediately rather than block. | 278 | # short read immediately rather than block. |
245 | 281 | bytes = self.read_bytes(_MAX_READ_SIZE) | 279 | bytes = self.read_bytes(osutils.MAX_SOCKET_CHUNK) |
246 | 282 | if bytes == '': | 280 | if bytes == '': |
247 | 283 | self.finished = True | 281 | self.finished = True |
248 | 284 | return | 282 | return |
249 | @@ -287,13 +285,13 @@ | |||
250 | 287 | self._push_back(protocol.unused_data) | 285 | self._push_back(protocol.unused_data) |
251 | 288 | 286 | ||
252 | 289 | def _read_bytes(self, desired_count): | 287 | def _read_bytes(self, desired_count): |
255 | 290 | return _read_bytes_from_socket( | 288 | return osutils.read_bytes_from_socket( |
256 | 291 | self.socket.recv, desired_count, self._report_activity) | 289 | self.socket, self._report_activity) |
257 | 292 | 290 | ||
258 | 293 | def terminate_due_to_error(self): | 291 | def terminate_due_to_error(self): |
259 | 294 | # TODO: This should log to a server log file, but no such thing | 292 | # TODO: This should log to a server log file, but no such thing |
260 | 295 | # exists yet. Andrew Bennetts 2006-09-29. | 293 | # exists yet. Andrew Bennetts 2006-09-29. |
262 | 296 | osutils.until_no_eintr(self.socket.close) | 294 | self.socket.close() |
263 | 297 | self.finished = True | 295 | self.finished = True |
264 | 298 | 296 | ||
265 | 299 | def _write_out(self, bytes): | 297 | def _write_out(self, bytes): |
266 | @@ -334,27 +332,27 @@ | |||
267 | 334 | bytes_to_read = protocol.next_read_size() | 332 | bytes_to_read = protocol.next_read_size() |
268 | 335 | if bytes_to_read == 0: | 333 | if bytes_to_read == 0: |
269 | 336 | # Finished serving this request. | 334 | # Finished serving this request. |
271 | 337 | osutils.until_no_eintr(self._out.flush) | 335 | self._out.flush() |
272 | 338 | return | 336 | return |
273 | 339 | bytes = self.read_bytes(bytes_to_read) | 337 | bytes = self.read_bytes(bytes_to_read) |
274 | 340 | if bytes == '': | 338 | if bytes == '': |
275 | 341 | # Connection has been closed. | 339 | # Connection has been closed. |
276 | 342 | self.finished = True | 340 | self.finished = True |
278 | 343 | osutils.until_no_eintr(self._out.flush) | 341 | self._out.flush() |
279 | 344 | return | 342 | return |
280 | 345 | protocol.accept_bytes(bytes) | 343 | protocol.accept_bytes(bytes) |
281 | 346 | 344 | ||
282 | 347 | def _read_bytes(self, desired_count): | 345 | def _read_bytes(self, desired_count): |
284 | 348 | return osutils.until_no_eintr(self._in.read, desired_count) | 346 | return self._in.read(desired_count) |
285 | 349 | 347 | ||
286 | 350 | def terminate_due_to_error(self): | 348 | def terminate_due_to_error(self): |
287 | 351 | # TODO: This should log to a server log file, but no such thing | 349 | # TODO: This should log to a server log file, but no such thing |
288 | 352 | # exists yet. Andrew Bennetts 2006-09-29. | 350 | # exists yet. Andrew Bennetts 2006-09-29. |
290 | 353 | osutils.until_no_eintr(self._out.close) | 351 | self._out.close() |
291 | 354 | self.finished = True | 352 | self.finished = True |
292 | 355 | 353 | ||
293 | 356 | def _write_out(self, bytes): | 354 | def _write_out(self, bytes): |
295 | 357 | osutils.until_no_eintr(self._out.write, bytes) | 355 | self._out.write(bytes) |
296 | 358 | 356 | ||
297 | 359 | 357 | ||
298 | 360 | class SmartClientMediumRequest(object): | 358 | class SmartClientMediumRequest(object): |
299 | @@ -711,6 +709,10 @@ | |||
300 | 711 | """A client medium using simple pipes. | 709 | """A client medium using simple pipes. |
301 | 712 | 710 | ||
302 | 713 | This client does not manage the pipes: it assumes they will always be open. | 711 | This client does not manage the pipes: it assumes they will always be open. |
303 | 712 | |||
304 | 713 | Note that if readable_pipe.read might raise IOError or OSError with errno | ||
305 | 714 | of EINTR, it must be safe to retry the read. Plain CPython fileobjects | ||
306 | 715 | (such as used for sys.stdin) are safe. | ||
307 | 714 | """ | 716 | """ |
308 | 715 | 717 | ||
309 | 716 | def __init__(self, readable_pipe, writeable_pipe, base): | 718 | def __init__(self, readable_pipe, writeable_pipe, base): |
310 | @@ -720,12 +722,12 @@ | |||
311 | 720 | 722 | ||
312 | 721 | def _accept_bytes(self, bytes): | 723 | def _accept_bytes(self, bytes): |
313 | 722 | """See SmartClientStreamMedium.accept_bytes.""" | 724 | """See SmartClientStreamMedium.accept_bytes.""" |
315 | 723 | osutils.until_no_eintr(self._writeable_pipe.write, bytes) | 725 | self._writeable_pipe.write(bytes) |
316 | 724 | self._report_activity(len(bytes), 'write') | 726 | self._report_activity(len(bytes), 'write') |
317 | 725 | 727 | ||
318 | 726 | def _flush(self): | 728 | def _flush(self): |
319 | 727 | """See SmartClientStreamMedium._flush().""" | 729 | """See SmartClientStreamMedium._flush().""" |
321 | 728 | osutils.until_no_eintr(self._writeable_pipe.flush) | 730 | self._writeable_pipe.flush() |
322 | 729 | 731 | ||
323 | 730 | def _read_bytes(self, count): | 732 | def _read_bytes(self, count): |
324 | 731 | """See SmartClientStreamMedium._read_bytes.""" | 733 | """See SmartClientStreamMedium._read_bytes.""" |
325 | @@ -777,15 +779,15 @@ | |||
326 | 777 | def _accept_bytes(self, bytes): | 779 | def _accept_bytes(self, bytes): |
327 | 778 | """See SmartClientStreamMedium.accept_bytes.""" | 780 | """See SmartClientStreamMedium.accept_bytes.""" |
328 | 779 | self._ensure_connection() | 781 | self._ensure_connection() |
330 | 780 | osutils.until_no_eintr(self._write_to.write, bytes) | 782 | self._write_to.write(bytes) |
331 | 781 | self._report_activity(len(bytes), 'write') | 783 | self._report_activity(len(bytes), 'write') |
332 | 782 | 784 | ||
333 | 783 | def disconnect(self): | 785 | def disconnect(self): |
334 | 784 | """See SmartClientMedium.disconnect().""" | 786 | """See SmartClientMedium.disconnect().""" |
335 | 785 | if not self._connected: | 787 | if not self._connected: |
336 | 786 | return | 788 | return |
339 | 787 | osutils.until_no_eintr(self._read_from.close) | 789 | self._read_from.close() |
340 | 788 | osutils.until_no_eintr(self._write_to.close) | 790 | self._write_to.close() |
341 | 789 | self._ssh_connection.close() | 791 | self._ssh_connection.close() |
342 | 790 | self._connected = False | 792 | self._connected = False |
343 | 791 | 793 | ||
344 | @@ -814,7 +816,7 @@ | |||
345 | 814 | if not self._connected: | 816 | if not self._connected: |
346 | 815 | raise errors.MediumNotConnected(self) | 817 | raise errors.MediumNotConnected(self) |
347 | 816 | bytes_to_read = min(count, _MAX_READ_SIZE) | 818 | bytes_to_read = min(count, _MAX_READ_SIZE) |
349 | 817 | bytes = osutils.until_no_eintr(self._read_from.read, bytes_to_read) | 819 | bytes = self._read_from.read(bytes_to_read) |
350 | 818 | self._report_activity(len(bytes), 'read') | 820 | self._report_activity(len(bytes), 'read') |
351 | 819 | return bytes | 821 | return bytes |
352 | 820 | 822 | ||
353 | @@ -844,7 +846,7 @@ | |||
354 | 844 | """See SmartClientMedium.disconnect().""" | 846 | """See SmartClientMedium.disconnect().""" |
355 | 845 | if not self._connected: | 847 | if not self._connected: |
356 | 846 | return | 848 | return |
358 | 847 | osutils.until_no_eintr(self._socket.close) | 849 | self._socket.close() |
359 | 848 | self._socket = None | 850 | self._socket = None |
360 | 849 | self._connected = False | 851 | self._connected = False |
361 | 850 | 852 | ||
362 | @@ -898,8 +900,8 @@ | |||
363 | 898 | """See SmartClientMedium.read_bytes.""" | 900 | """See SmartClientMedium.read_bytes.""" |
364 | 899 | if not self._connected: | 901 | if not self._connected: |
365 | 900 | raise errors.MediumNotConnected(self) | 902 | raise errors.MediumNotConnected(self) |
368 | 901 | return _read_bytes_from_socket( | 903 | return osutils.read_bytes_from_socket( |
369 | 902 | self._socket.recv, count, self._report_activity) | 904 | self._socket, self._report_activity) |
370 | 903 | 905 | ||
371 | 904 | 906 | ||
372 | 905 | class SmartClientStreamMediumRequest(SmartClientMediumRequest): | 907 | class SmartClientStreamMediumRequest(SmartClientMediumRequest): |
373 | @@ -942,19 +944,3 @@ | |||
374 | 942 | self._medium._flush() | 944 | self._medium._flush() |
375 | 943 | 945 | ||
376 | 944 | 946 | ||
377 | 945 | def _read_bytes_from_socket(sock, desired_count, report_activity): | ||
378 | 946 | # We ignore the desired_count because on sockets it's more efficient to | ||
379 | 947 | # read large chunks (of _MAX_READ_SIZE bytes) at a time. | ||
380 | 948 | try: | ||
381 | 949 | bytes = osutils.until_no_eintr(sock, _MAX_READ_SIZE) | ||
382 | 950 | except socket.error, e: | ||
383 | 951 | if len(e.args) and e.args[0] in (errno.ECONNRESET, 10054): | ||
384 | 952 | # The connection was closed by the other side. Callers expect an | ||
385 | 953 | # empty string to signal end-of-stream. | ||
386 | 954 | bytes = '' | ||
387 | 955 | else: | ||
388 | 956 | raise | ||
389 | 957 | else: | ||
390 | 958 | report_activity(len(bytes), 'read') | ||
391 | 959 | return bytes | ||
392 | 960 | |||
393 | 961 | 947 | ||
394 | === modified file 'bzrlib/smart/protocol.py' | |||
395 | --- bzrlib/smart/protocol.py 2010-02-17 17:11:16 +0000 | |||
396 | +++ bzrlib/smart/protocol.py 2010-03-26 04:50:44 +0000 | |||
397 | @@ -62,7 +62,13 @@ | |||
398 | 62 | 62 | ||
399 | 63 | def _encode_tuple(args): | 63 | def _encode_tuple(args): |
400 | 64 | """Encode the tuple args to a bytestream.""" | 64 | """Encode the tuple args to a bytestream.""" |
402 | 65 | return '\x01'.join(args) + '\n' | 65 | joined = '\x01'.join(args) + '\n' |
403 | 66 | if type(joined) is unicode: | ||
404 | 67 | # XXX: We should fix things so this never happens! -AJB, 20100304 | ||
405 | 68 | mutter('response args contain unicode, should be only bytes: %r', | ||
406 | 69 | joined) | ||
407 | 70 | joined = joined.encode('ascii') | ||
408 | 71 | return joined | ||
409 | 66 | 72 | ||
410 | 67 | 73 | ||
411 | 68 | class Requester(object): | 74 | class Requester(object): |
412 | 69 | 75 | ||
413 | === modified file 'bzrlib/smart/server.py' | |||
414 | --- bzrlib/smart/server.py 2010-02-23 07:43:11 +0000 | |||
415 | +++ bzrlib/smart/server.py 2010-03-26 04:50:44 +0000 | |||
416 | @@ -138,6 +138,8 @@ | |||
417 | 138 | if e.args[0] != errno.EBADF: | 138 | if e.args[0] != errno.EBADF: |
418 | 139 | trace.warning("listening socket error: %s", e) | 139 | trace.warning("listening socket error: %s", e) |
419 | 140 | else: | 140 | else: |
420 | 141 | if self._should_terminate: | ||
421 | 142 | break | ||
422 | 141 | self.serve_conn(conn, thread_name_suffix) | 143 | self.serve_conn(conn, thread_name_suffix) |
423 | 142 | except KeyboardInterrupt: | 144 | except KeyboardInterrupt: |
424 | 143 | # dont log when CTRL-C'd. | 145 | # dont log when CTRL-C'd. |
nice