Merge lp:~spiv/bzr/ssh-connect-socket-error into lp:bzr

[Andrew Bennetts]

Simplify connect_ssh/sftp error handling

test_bad_ssh_connection is still failing intermittently: http://babune.ladeuil.net:24842/job/selftest-hardy/lastFailedBuild/testReport/junit/bzrlib.tests.test_sftp_transport/SSHVendorBadConnection/test_bad_connection_ssh/

Looking at the code, I don't know what the separate handling of EPIPE is trying to achieve, so here's a patch to just keep the error handling during connect_ssh/sftp simple and consistent.

[Vincent Ladeuil (vila)]

Let's try that and monitor the failures on hardy.

AIUI this shouldn't occur outside of tests so is pretty safe to try.

review: Approve

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> Review: Approve
> Let's try that and monitor the failures on hardy.
>
> AIUI this shouldn't occur outside of tests so is pretty safe to try.

I'm not as confident about that, but I think I've forgotten too much of
the original context to be confident of anything here :/

[Vincent Ladeuil (vila)]

At least this didn't fail any tests on: http://babune.ladeuil.net:24842/view/debug/job/selftest-subset-all/lastCompletedBuild/aggregatedTestReport/ for -s bzrlib.tests.test_sftp_transport

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-09-15 10:10:22 +0000
+++ NEWS	2010-09-17 07:43:44 +0000
@@ -183,6 +183,9 @@
   be available from ``bzrlib.tests.*``. They used to be, but were
   accidentally removed. (John Arbash Meinel, #627438)
 
+* Treat all IO, OS, and socket errors consistently when establishing
+  SSH/SFTP connections via a subprocess.  (Andrew Bennetts)
+
 * ``Transport.stat`` on a symlink, including a transport pointing directly
   to a symlink, now returns information about the symlink.
   (Martin Pool)

=== modified file 'bzrlib/transport/ssh.py'
--- bzrlib/transport/ssh.py	2010-09-15 10:10:22 +0000
+++ bzrlib/transport/ssh.py	2010-09-17 07:43:44 +0000
@@ -336,15 +336,14 @@
             self._raise_connection_error(host, port=port, orig_error=e,
                                          msg='Unable to invoke remote bzr')
 
+_ssh_connection_errors = (EOFError, OSError, IOError, socket.error)
 if paramiko is not None:
     vendor = ParamikoVendor()
     register_ssh_vendor('paramiko', vendor)
     register_ssh_vendor('none', vendor)
     register_default_ssh_vendor(vendor)
-    _sftp_connection_errors = (EOFError, paramiko.SSHException)
+    _ssh_connection_errors += (paramiko.SSHException,)
     del vendor
-else:
-    _sftp_connection_errors = (EOFError,)
 
 
 class SubprocessVendor(SSHVendor):
@@ -375,14 +374,7 @@
                                                   subsystem='sftp')
             sock = self._connect(argv)
             return SFTPClient(SocketAsChannelAdapter(sock))
-        except _sftp_connection_errors, e:
-            self._raise_connection_error(host, port=port, orig_error=e)
-        except (OSError, IOError, socket.error), e:
-            # If the machine is fast enough, ssh can actually exit
-            # before we try and send it the sftp request, which
-            # raises a Broken Pipe
-            if e.errno not in (errno.EPIPE,):
-                raise
+        except _ssh_connection_errors, e:
             self._raise_connection_error(host, port=port, orig_error=e)
 
     def connect_ssh(self, username, password, host, port, command):
@@ -390,14 +382,7 @@
             argv = self._get_vendor_specific_argv(username, host, port,
                                                   command=command)
             return self._connect(argv)
-        except (EOFError), e:
-            self._raise_connection_error(host, port=port, orig_error=e)
-        except (OSError, IOError, socket.error), e:
-            # If the machine is fast enough, ssh can actually exit
-            # before we try and send it the sftp request, which
-            # raises a Broken Pipe
-            if e.errno not in (errno.EPIPE,):
-                raise
+        except _ssh_connection_errors, e:
             self._raise_connection_error(host, port=port, orig_error=e)
 
     def _get_vendor_specific_argv(self, username, host, port, subsystem=None,