Merge lp:~jameinel/bzr/2.5-unending-sendall-1047309 into lp:bzr/2.5

[John A Meinel]

Detect if we are not progressing during osutils.send_all() and fail rather than hang. (bug #1047309)

See the associated bug #1047309.

Basically, with the new ConnectionReset code coming into bzr-2.5, it seems that if Paramiko gets disconnected, it is unable to send any content, but doesn't raise an error. (it returns 0 bytes sent, but no exception is raised.)

As a conservative fix, this allows us to get a series of up to 5 0-bytes-sent, but any more than that in a row, and we just treat it as a ECONNRESET. Looking at the man pages, it seems send in non-blocking socket shouldn't ever give 0 bytes sent, but I preferred to be a little cautious.

If this causes fallout, we could tweak the retries, or even do sleep-based backoff.

[Martin Packman (gz)]

Did you find any answer on if it's valid for send() to return 0? As far as I'm aware it's not, it's either EINTR or a partial write of more than one byte, though I can't find anything explicit either.

For implementations like paramiko that just emulate the interface, coping with this is I guess required, though it would be nice to fix upstream to do something more sane. I'd prefer raising an error straight off, but understand the motivation behind retying several times, as that would previously have worked and regressions are bad. I'd much prefer not to use this heuristic if at possible though, instead quirking on the socket type or something that makes it explict this is a paramiko issue only.

review: Needs Information

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 9/7/2012 7:54 PM, Martin Packman wrote:
> Review: Needs Information
>
> Did you find any answer on if it's valid for send() to return 0? As
> far as I'm aware it's not, it's either EINTR or a partial write of
> more than one byte, though I can't find anything explicit either.
>
> For implementations like paramiko that just emulate the interface,
> coping with this is I guess required, though it would be nice to
> fix upstream to do something more sane. I'd prefer raising an error
> straight off, but understand the motivation behind retying several
> times, as that would previously have worked and regressions are
> bad. I'd much prefer not to use this heuristic if at possible
> though, instead quirking on the socket type or something that makes
> it explict this is a paramiko issue only.
>

Well, we could go with immediately erroring, and see how bad the
fallout is...

I think this is a reasonable compromise which means we won't have to
think about it again.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlBLLYMACgkQJdeBCYSNAAPMXwCdHdkRL+i8RzI7PckS7yFce62M
1NIAnjiQ3YrIyh7xgkifk5JIScAN05YS
=zHgy
-----END PGP SIGNATURE-----

[Martin Packman (gz)]

Won't have to think about, apart from when someone writing code wants to know if they should use socket.sendall or osutils.sendall and what the hell is going on... So, I don't really like the code but am somewhat sold on it being the least risky option for fixing this bug.

Suggested tweaks before landing:
* Remove note about making _max_no_content_sends configurable, internal hacks like this shouldn't be exposed
* Add comment/docstring to osutils.sendall about why it's checking for 0 from send
* Maybe add mutter if we get 0 returned from send which is probably the only way we'll find out if it gets hit in the wild
* Make test_send_minimal_progress clear that this is not behaviour we've actually observed

review: Approve

[Richard Wilbur (richard-wilbur)]

The documentation for paramiko.Channel.send: "Returns the number of bytes sent, or 0 if the channel stream is closed."[1]

Python 2.7 socket library documentation for socket.send: "Returns the number of bytes sent. Applications are responsible for checking that all data has been sent; if only some of the data was transmitted, the application needs to attempt delivery of the remaining data. For further information on this concept, consult the Socket Programming HOWTO."[2]

Interestingly the Socket Programming HOWTO (referenced above) has an example which suggests you might get a 0 return value[3], but only if the connection is broken:
            sent = self.sock.send(msg[totalsent:])
            if sent == 0:
                raise RuntimeError("socket connection broken")

Linux documentation for socket send[4]:
RETURN VALUE
       On success, these calls return the number of characters sent. On
       error, -1 is returned, and errno is set appropriately.

The consensus seems to be that a socket send should never return 0 unless there is an error. Paramiko looks like it thinks that is a normal way to signal a closed connection. The python socket programming HOWTO implies that the only reason for a 0 return value is also a closed connection.

@John, I agree with your proposition that we should throw an exception immediately when send returns 0.
@Martin, this would alleviate the need for _max_no_content_sends. Hence,
  1. I am in favour of removing _max_no_content_sends
  2. Comment/docstring for osutils.sendall would be fine if it isn't obvious from the exception we throw
  3.,4. Agree

References:
[1] http://www.lag.net/paramiko/docs/paramiko.Channel-class.html#send
[2] http://docs.python.org/library/socket.html
[3] http://docs.python.org/howto/sockets.html#socket-howto
[4] info send

review: Needs Fixing

[John A Meinel (jameinel)]

Code has been simplified to just treat 0 bytes as closed connection.

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2012-02-28 04:58:14 +0000
+++ bzrlib/osutils.py	2012-09-11 07:29:18 +0000
@@ -2140,8 +2140,12 @@
             if e.args[0] != errno.EINTR:
                 raise
         else:
+            if sent == 0:
+                raise errors.ConnectionReset('Sending to %s returned 0 bytes'
+                                             % (sock,))
             sent_total += sent
-            report_activity(sent, 'write')
+            if report_activity is not None:
+                report_activity(sent, 'write')
 
 
 def connect_socket(address):
 
=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2012-02-28 04:58:14 +0000
+++ bzrlib/tests/test_osutils.py	2012-09-11 07:29:18 +0000
@@ -819,6 +819,28 @@
         self.assertEqual(None, osutils.safe_file_id(None))
 
 
+
+class TestSendAll(tests.TestCase):
+
+    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,
+        # but it returns 0 bytes sent for requests over and over again.
+        class NoSendingSocket(object):
+            def __init__(self):
+                self.call_count = 0
+            def send(self, bytes):
+                self.call_count += 1
+                if self.call_count > 100:
+                    # Prevent the test suite from hanging
+                    raise RuntimeError('too many calls')
+                return 0
+        sock = NoSendingSocket()
+        self.assertRaises(errors.ConnectionReset,
+                          osutils.send_all, sock, 'content')
+        self.assertEqual(1, sock.call_count)
+
+
 class TestPosixFuncs(tests.TestCase):
     """Test that the posix version of normpath returns an appropriate path
        when used with 2 leading slashes."""
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-09-06 12:05:27 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-09-11 07:29:18 +0000
@@ -39,6 +39,13 @@
   extracted texts from the repository. (Just an ordering constraint on how
   they consumed the stream.) (John Arbash Meinel, #1046284)
 
+* ``osutils.send_all`` now detects if we get a series of zero bytes sent,
+  and fails with a ECONNRESET. It seems if paramiko gets disconnected, it
+  will get into a state where it returns 0 bytes sent, but doesn't raise
+  an error. This change allows us to get a couple hiccups of no content
+  sent, but if it is consistent, we will consider it to be a failure.
+  (John Arbash Meinel, #1047309)
+
 * Revert use of --no-tty when gpg signing commits. (Jelmer Vernooij, #1014570)
 
 * Some small bug fixes wrt lightweight checkouts and remote repositories.