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

[John Arbash Meinel]

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
> +            else:
> +                warning("FTP connection closed. Trying to reopen.")
> +                self._FTP_instance = None
> +                return self.get(relpath, decode, retries+1)
> +
> +    def put(self, relpath, fp, mode=None, retries=0):
>          """Copy the file-like or string object into the location.
>  
>          :param relpath: Location to put the contents, relative to base.
> @@ -208,11 +222,25 @@
>              f.storbinary('STOR '+self._abspath(relpath), fp, 8192)
>          except ftplib.error_perm, e:
>              if "no such file" in str(e).lower():
> -                raise NoSuchFile(msg="Error storing %s: %s"
> -                                 % (self.abspath(relpath), str(e)),
> -                                 orig_error=e)
> +                raise NoSuchFile("Error storing %s: %s"
> +                                 % (self.abspath(relpath), str(e)))

NoSuchFile expects a path as the first argument, and an optional extra
information as the second (this can just be the exception object).
So it should be:

NoSuchFile(self.abspath(relpath), extra=e)

I realize you didn't make it wrong, but we should correct it now.


>              else:
>                  raise FtpTransportError(orig_error=e)
> +        except ftplib.error_temp, e:
> +            if retries > 1:
> +                raise e
> +            else:
> +                warning("FTP temporary error: %s. Retrying." % str(e))
> +                self._FTP_instance = None
> +                self.put(relpath, fp, mode, retries+1)
> +        except EOFError, e:
> +            if retries > 1:
> +                raise e
> +            else:
> +                warning("FTP connection closed. Trying to reopen.")
> +                self._FTP_instance = None
> +                self.put(relpath, fp, mode, retries+1)
> +
>  
>      def mkdir(self, relpath, mode=None):
>          """Create a directory at the given path."""
> 
> 

With 'retries' are you trying to say how many times you should retry, or
that this is the 'nth' retry?

Since it seems to be the latter, I think the word 'retry' makes more sense.

Also, I would add documentation to the get() and put() docstring to make
it clean what the parameters mean.

Also, we should probably factor out the '1' as a constant... Something like:

_number_of_retries = 1

class FTPTransport(Transport):
...

  if retries > _number_of_retries:
    raise

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 252 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060129/26c25ec7/attachment.pgp