[RFC][PATCH] ftp transport, dsilver's branch

[Robert Collins]

On Sat, 2006-01-28 at 22:29 +0100, Matthieu Moy wrote:
> Matthieu Moy <Matthieu.Moy at imag.fr> writes:
> 
> > Hi,
> >
> > First, a reminder: Daniel Silverstone's ftp branch contains some fixes
> > that have not been merged. In particular, it allows reusing
> > connections instead of creating several ones in parallel[1].
> 
> Followup to myself, with another patch:
> 
> It tries to solve the problem of ftp server closing the connection
> arbitrarily after some time. The patch works for me (I could push a
> branch of 849 revisions with 6 temporary failures, but it did it in
> one go), but it probably needs to be improved before getting into
> upstream. Daniel, I suggest that you merge it into your branch first.
> 
> Hopefully, a branch containing this fix will be available here within
> a few minutes (I'm going to bed, so let's see if my patch still works
> while I'm sleeping):
> 
>   http://test.bzr.free.fr/branches/bzr.ftp/
> 
> The patch (against dsilver's branch) is below:
> 
> === modified file 'bzrlib/transport/ftp.py'
> --- bzrlib/transport/ftp.py     
> +++ bzrlib/transport/ftp.py     
> @@ -37,7 +37,7 @@
>  from bzrlib.transport import Transport
>  from bzrlib.errors import (TransportNotPossible, TransportError,
>                             NoSuchFile, FileExists)
> -from bzrlib.trace import mutter
> +from bzrlib.trace import mutter, warning
>  
> 
>  _FTP_cache = {}
> @@ -174,7 +174,7 @@
>              mutter("FTP has not: %s" % self._abspath(relpath))
>              return False
>  
> -    def get(self, relpath, decode=False):
> +    def get(self, relpath, decode=False, retries=0):
>          """Get the file at the given relative path.
>  
>          :param relpath: The relative path to the file
> @@ -190,9 +190,23 @@
>              ret.seek(0)
>              return ret
>          except ftplib.error_perm, e:
> -            raise NoSuchFile(self.abspath(relpath), extra=extra)
> -
> -    def put(self, relpath, fp, mode=None):
> +            raise NoSuchFile(self.abspath(relpath))
> +        except ftplib.error_temp, e:
> +            if retries > 1:
> +                raise e
> +            else:
> +                warning("FTP temporary error: %s. Retrying." % str(e))
> +                self._FTP_instance = None
> +                return self.get(relpath, decode, retries+1)
> +        except EOFError, e:
> +            if retries > 1:
> +                raise e

Rather than 'raise e' which destroys the current backtrace, just use
'raise'.

> +            else:
> +                warning("FTP connection closed. Trying to reopen.")
> +                self._FTP_instance = None
> +                return self.get(relpath, decode, retries+1)

I think it should mention 'control' in that message - as reopening a
transfer connection would be liable to introduce data races.

Other than that, it seems straight forward - +1 with those fixed
(throughout the patch)

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060130/8e201e07/attachment.pgp