Bazaar

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

  1. ssh-connect-socket-error
  2. Merge into bzr.dev
Proposed by Andrew Bennetts
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5438
Proposed branch: lp:~spiv/bzr/ssh-connect-socket-error
Merge into: lp:bzr
Diff against target: 67 lines (+7/-19)
2 files modified
NEWS (+3/-0)
bzrlib/transport/ssh.py (+4/-19)
To merge this branch: bzr merge lp:~spiv/bzr/ssh-connect-socket-error
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Review via email: mp+35784@code.launchpad.net

Commit message

Simplify connect_ssh/sftp error handling

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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
Revision history for this message
Andrew Bennetts (spiv) wrote :

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 :/

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-09-15 10:10:22 +0000
+++ NEWS 2010-09-17 07:43:44 +0000
@@ -183,6 +183,9 @@
183 be available from ``bzrlib.tests.*``. They used to be, but were183 be available from ``bzrlib.tests.*``. They used to be, but were
184 accidentally removed. (John Arbash Meinel, #627438)184 accidentally removed. (John Arbash Meinel, #627438)
185185
186* Treat all IO, OS, and socket errors consistently when establishing
187 SSH/SFTP connections via a subprocess. (Andrew Bennetts)
188
186* ``Transport.stat`` on a symlink, including a transport pointing directly189* ``Transport.stat`` on a symlink, including a transport pointing directly
187 to a symlink, now returns information about the symlink.190 to a symlink, now returns information about the symlink.
188 (Martin Pool)191 (Martin Pool)
189192
=== 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 @@
336 self._raise_connection_error(host, port=port, orig_error=e,336 self._raise_connection_error(host, port=port, orig_error=e,
337 msg='Unable to invoke remote bzr')337 msg='Unable to invoke remote bzr')
338338
339_ssh_connection_errors = (EOFError, OSError, IOError, socket.error)
339if paramiko is not None:340if paramiko is not None:
340 vendor = ParamikoVendor()341 vendor = ParamikoVendor()
341 register_ssh_vendor('paramiko', vendor)342 register_ssh_vendor('paramiko', vendor)
342 register_ssh_vendor('none', vendor)343 register_ssh_vendor('none', vendor)
343 register_default_ssh_vendor(vendor)344 register_default_ssh_vendor(vendor)
344 _sftp_connection_errors = (EOFError, paramiko.SSHException)345 _ssh_connection_errors += (paramiko.SSHException,)
345 del vendor346 del vendor
346else:
347 _sftp_connection_errors = (EOFError,)
348347
349348
350class SubprocessVendor(SSHVendor):349class SubprocessVendor(SSHVendor):
@@ -375,14 +374,7 @@
375 subsystem='sftp')374 subsystem='sftp')
376 sock = self._connect(argv)375 sock = self._connect(argv)
377 return SFTPClient(SocketAsChannelAdapter(sock))376 return SFTPClient(SocketAsChannelAdapter(sock))
378 except _sftp_connection_errors, e:377 except _ssh_connection_errors, e:
379 self._raise_connection_error(host, port=port, orig_error=e)
380 except (OSError, IOError, socket.error), e:
381 # If the machine is fast enough, ssh can actually exit
382 # before we try and send it the sftp request, which
383 # raises a Broken Pipe
384 if e.errno not in (errno.EPIPE,):
385 raise
386 self._raise_connection_error(host, port=port, orig_error=e)378 self._raise_connection_error(host, port=port, orig_error=e)
387379
388 def connect_ssh(self, username, password, host, port, command):380 def connect_ssh(self, username, password, host, port, command):
@@ -390,14 +382,7 @@
390 argv = self._get_vendor_specific_argv(username, host, port,382 argv = self._get_vendor_specific_argv(username, host, port,
391 command=command)383 command=command)
392 return self._connect(argv)384 return self._connect(argv)
393 except (EOFError), e:385 except _ssh_connection_errors, e:
394 self._raise_connection_error(host, port=port, orig_error=e)
395 except (OSError, IOError, socket.error), e:
396 # If the machine is fast enough, ssh can actually exit
397 # before we try and send it the sftp request, which
398 # raises a Broken Pipe
399 if e.errno not in (errno.EPIPE,):
400 raise
401 self._raise_connection_error(host, port=port, orig_error=e)386 self._raise_connection_error(host, port=port, orig_error=e)
402387
403 def _get_vendor_specific_argv(self, username, host, port, subsystem=None,388 def _get_vendor_specific_argv(self, username, host, port, subsystem=None,