Merge lp:~jameinel/bzr/2.5-conn-reset-socket-pipe-1047325 into lp:bzr/2.5

Proposed by John A Meinel
Status: Merged
Approved by: John A Meinel
Approved revision: 6514
Merged at revision: 6511
Proposed branch: lp:~jameinel/bzr/2.5-conn-reset-socket-pipe-1047325
Merge into: lp:bzr/2.5
Diff against target: 78 lines (+25/-5)
3 files modified
bzrlib/osutils.py (+5/-2)
bzrlib/smart/medium.py (+2/-2)
bzrlib/tests/test_osutils.py (+18/-1)
To merge this branch: bzr merge lp:~jameinel/bzr/2.5-conn-reset-socket-pipe-1047325
Reviewer Review Type Date Requested Status
Richard Wilbur Approve
Review via email: mp+123535@code.launchpad.net

Commit message

Treat socket.error(EPIPE) as a ConnectionReset (bug #1047325) so that clients will properly reconnect.

Description of the change

This updates osutils.send_all to translate more failures into errors.ConnectionReset.

I was originally going to do it at the higher level, but then I realized nobody who uses send_all really wants to be handling the raw IOError/socket.error sort of objects.
This will conflict a bit with my other osutils.send_all patch, but I'll handle resolving the conflicts.

To post a comment you must log in.
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

This looks good but I agree the integration with your other patch is very important. Should the other patch also treat a single 0 return value as a ConnectionReset?

+1

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

Yes, I did change the other branch in the same way.

6514. By John A Meinel

Merge 2.5 including the other osutils.send_all fix.

Revision history for this message
John A Meinel (jameinel) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/osutils.py'
2--- bzrlib/osutils.py 2012-09-11 07:26:51 +0000
3+++ bzrlib/osutils.py 2012-09-11 08:42:19 +0000
4@@ -2064,7 +2064,7 @@
5 # data at once.
6 MAX_SOCKET_CHUNK = 64 * 1024
7
8-_end_of_stream_errors = [errno.ECONNRESET]
9+_end_of_stream_errors = [errno.ECONNRESET, errno.EPIPE, errno.EINVAL]
10 for _eno in ['WSAECONNRESET', 'WSAECONNABORTED']:
11 _eno = getattr(errno, _eno, None)
12 if _eno is not None:
13@@ -2136,7 +2136,10 @@
14 while sent_total < byte_count:
15 try:
16 sent = sock.send(buffer(bytes, sent_total, MAX_SOCKET_CHUNK))
17- except socket.error, e:
18+ except (socket.error, IOError), e:
19+ if e.args[0] in _end_of_stream_errors:
20+ raise errors.ConnectionReset(
21+ "Error trying to write to socket", e)
22 if e.args[0] != errno.EINTR:
23 raise
24 else:
25
26=== modified file 'bzrlib/smart/medium.py'
27--- bzrlib/smart/medium.py 2011-12-24 09:59:02 +0000
28+++ bzrlib/smart/medium.py 2012-09-11 08:42:19 +0000
29@@ -910,7 +910,7 @@
30 except IOError, e:
31 if e.errno in (errno.EINVAL, errno.EPIPE):
32 raise errors.ConnectionReset(
33- "Error trying to write to subprocess:\n%s" % (e,))
34+ "Error trying to write to subprocess", e)
35 raise
36 self._report_activity(len(bytes), 'write')
37
38@@ -1043,7 +1043,7 @@
39
40 class SmartClientSocketMedium(SmartClientStreamMedium):
41 """A client medium using a socket.
42-
43+
44 This class isn't usable directly. Use one of its subclasses instead.
45 """
46
47
48=== modified file 'bzrlib/tests/test_osutils.py'
49--- bzrlib/tests/test_osutils.py 2012-09-11 07:26:51 +0000
50+++ bzrlib/tests/test_osutils.py 2012-09-11 08:42:19 +0000
51@@ -819,9 +819,26 @@
52 self.assertEqual(None, osutils.safe_file_id(None))
53
54
55-
56 class TestSendAll(tests.TestCase):
57
58+ def test_send_with_disconnected_socket(self):
59+ class DisconnectedSocket(object):
60+ def __init__(self, err):
61+ self.err = err
62+ def send(self, content):
63+ raise self.err
64+ def close(self):
65+ pass
66+ # All of these should be treated as ConnectionReset
67+ errs = []
68+ for err_cls in (IOError, socket.error):
69+ for errnum in osutils._end_of_stream_errors:
70+ errs.append(err_cls(errnum))
71+ for err in errs:
72+ sock = DisconnectedSocket(err)
73+ self.assertRaises(errors.ConnectionReset,
74+ osutils.send_all, sock, 'some more content')
75+
76 def test_send_with_no_progress(self):
77 # See https://bugs.launchpad.net/bzr/+bug/1047309
78 # It seems that paramiko can get into a state where it doesn't error,

Subscribers

People subscribed via source and target branches