[RFC][PATCH] ftp transport, dsilver's branch
John Arbash Meinel
john at arbash-meinel.com
Mon Jan 30 00:53:24 GMT 2006
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
More information about the bazaar
mailing list