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

[John A Meinel]

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

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.

[Richard Wilbur (richard-wilbur)]

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

[John A Meinel (jameinel)]

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

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2012-09-11 07:26:51 +0000
+++ bzrlib/osutils.py	2012-09-11 08:42:19 +0000
@@ -2064,7 +2064,7 @@
 # data at once.
 MAX_SOCKET_CHUNK = 64 * 1024
 
-_end_of_stream_errors = [errno.ECONNRESET]
+_end_of_stream_errors = [errno.ECONNRESET, errno.EPIPE, errno.EINVAL]
 for _eno in ['WSAECONNRESET', 'WSAECONNABORTED']:
     _eno = getattr(errno, _eno, None)
     if _eno is not None:
@@ -2136,7 +2136,10 @@
     while sent_total < byte_count:
         try:
             sent = sock.send(buffer(bytes, sent_total, MAX_SOCKET_CHUNK))
-        except socket.error, e:
+        except (socket.error, IOError), e:
+            if e.args[0] in _end_of_stream_errors:
+                raise errors.ConnectionReset(
+                    "Error trying to write to socket", e)
             if e.args[0] != errno.EINTR:
                 raise
         else:

=== modified file 'bzrlib/smart/medium.py'
--- bzrlib/smart/medium.py	2011-12-24 09:59:02 +0000
+++ bzrlib/smart/medium.py	2012-09-11 08:42:19 +0000
@@ -910,7 +910,7 @@
         except IOError, e:
             if e.errno in (errno.EINVAL, errno.EPIPE):
                 raise errors.ConnectionReset(
-                    "Error trying to write to subprocess:\n%s" % (e,))
+                    "Error trying to write to subprocess", e)
             raise
         self._report_activity(len(bytes), 'write')
 
@@ -1043,7 +1043,7 @@
 
 class SmartClientSocketMedium(SmartClientStreamMedium):
     """A client medium using a socket.
-    
+
     This class isn't usable directly.  Use one of its subclasses instead.
     """
 

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2012-09-11 07:26:51 +0000
+++ bzrlib/tests/test_osutils.py	2012-09-11 08:42:19 +0000
@@ -819,9 +819,26 @@
         self.assertEqual(None, osutils.safe_file_id(None))
 
 
-
 class TestSendAll(tests.TestCase):
 
+    def test_send_with_disconnected_socket(self):
+        class DisconnectedSocket(object):
+            def __init__(self, err):
+                self.err = err
+            def send(self, content):
+                raise self.err
+            def close(self):
+                pass
+        # All of these should be treated as ConnectionReset
+        errs = []
+        for err_cls in (IOError, socket.error):
+            for errnum in osutils._end_of_stream_errors:
+                errs.append(err_cls(errnum))
+        for err in errs:
+            sock = DisconnectedSocket(err)
+            self.assertRaises(errors.ConnectionReset,
+                osutils.send_all, sock, 'some more content')
+
     def test_send_with_no_progress(self):
         # See https://bugs.launchpad.net/bzr/+bug/1047309
         # It seems that paramiko can get into a state where it doesn't error,