Merge lp:~mterry/duplicity/webdav-fixes into lp:~duplicity-team/duplicity/0.7-series

[Michael Terry]

This branch fixes two issues I saw when testing the webdav backend:

1) Errors like the following: "Attempt 1 failed. BackendException: File /tmp/duplicity-LQ1a0i-tempdir/mktemp-u2aiyX-2 not found locally after get from backend"

These were caused by the _get() method not calling setdata() on the local path object, so the rest of the code thought it didn't exist.

2) Some odd issues from stale responses/data. We have a couple places in webdavbackend.py where we close the connection before making a request because of this problem. But I've changed it to do it every time, more reliably, by putting a _close() call inside the request() method.

With this, the webdav backend seems fine to me.

[edso (ed.so)]

On 15.06.2014 22:26, Michael Terry wrote:
> 2) Some odd issues from stale responses/data. We have a couple places in webdavbackend.py where we close the connection before making a request because of this problem. But I've changed it to do it every time, more reliably, by putting a _close() call inside the request() method.
>
> With this, the webdav backend seems fine to me.

sure you can do that. but it seems to me that the original author had a web background and didn't do it on purpose to reuse the already setup connection for performance reasons.

i would bet that now every request needs to be re-authenticated now. not sure that's a good idea.

..ede

PS: i wonder why the backend suddenly needs this other way to write the request data. any clue?

[Michael Terry (mterry)]

> i would bet that now every request needs to be re-authenticated now. not sure that's a good idea.

Well... We already did the connection close in many cases. I just made it consistent.

I feel like the connection close to clear out the bad data/status was a bit of hack in the first place -- seems like really odd behavior of the library, but the workaround worked well. I'm just extending the hack.

If we could come up with a better way that doesn't mean we close the connection, I'm all ears. But I'm not very familiar with http python libraries.

> PS: i wonder why the backend suddenly needs this other way to write the request data. any clue?

It was an extra check added during the backends re-organization to enforce correctness. When writing to a Path object, you ideally call setdata() on it after.

[edso (ed.so)]

On 15.06.2014 22:57, Michael Terry wrote:
>> i would bet that now every request needs to be re-authenticated now. not sure that's a good idea.
>
> Well... We already did the connection close in many cases. I just made it consistent.

nope, just when failure occured, of which the error cause might have been the connection, hence the reset.

> I feel like the connection close to clear out the bad data/status was a bit of hack in the first place -- seems like really odd behavior of the library, but the workaround worked well. I'm just extending the hack.

actually the workaround was to read the response, which seemed to be managed by the connection and not properly cleaned on response.close()

> If we could come up with a better way that doesn't mean we close the connection, I'm all ears.

yeah.. just read out the response every time and close/reset only on errors.

>But I'm not very familiar with http python libraries.

pfffh.. don't ask me. i did this stuff in php & perl once upon a time, but i assume the mechanics shouldn't be to different.

>> PS: i wonder why the backend suddenly needs this other way to write the request data. any clue?
>
> It was an extra check added during the backends re-organization to enforce correctness. When writing to a Path object, you ideally call setdata() on it after.
>

ohh, i see, so your added check was that actually failed. not the old code?

..ede

[lp:~mterry/duplicity/webdav-fixes]

992. By Michael Terry on 2014-06-17

Fix setdata() not being called a better way

[Michael Terry (mterry)]

> nope, just when failure occured, of which the error cause might have been the connection, hence the reset.

That's not quite true. We did it in a normal case and an error case. But I'm willing to believe there's a better fix. Let me try forcing a read...

> ohh, i see, so your added check was that actually failed. not the old code?

Yeah, I guess. Your comment made me rethink the fix. I forgot the mantra of the backend rewrite, which is "let backends be dumb". We already call setdata() for the backend, but we were doing it before the check I added.

Better to just move the setdata() call before the check. I've done that already in this branch.

[Michael Terry (mterry)]

OK, playing around with it, I couldn't make anything besides the close-every-request thing work. Try the following command:

./testing/manual/backendtest 'webdavs://demo:<email address hidden>/demo/duplicity-testing'

This fails in a few places for me without the close behavior in this tree. I'm sure there's a more elegant fix, but I haven't found it yet. Any ideas? Do you see the same bugs I do with the command?

[edso (ed.so)]

On 18.06.2014 00:03, Michael Terry wrote:
> Better to just move the setdata() call before the check. I've done that already in this branch.

what does setdata() do anyway? ..ede

[edso (ed.so)]

On 18.06.2014 02:31, Michael Terry wrote:
> OK, playing around with it, I couldn't make anything besides the close-every-request thing work. Try the following command:
>
> ./testing/manual/backendtest 'webdavs://demo:<email address hidden>/demo/duplicity-testing'
>
> This fails in a few places for me without the close behavior in this tree. I'm sure there's a more elegant fix, but I haven't found it yet. Any ideas? Do you see the same bugs I do with the command?
>

give me some time to look at it.. ede

[Michael Terry (mterry)]

> what does setdata() do anyway? ..ede

Basically just a stat() on the backing file for the Path object. Fills out the fields that tell if the file exists, is writable, etc.

=== modified file 'duplicity/backend.py'
--- duplicity/backend.py	2014-05-11 05:43:32 +0000
+++ duplicity/backend.py	2014-06-17 21:56:30 +0000
@@ -538,10 +538,10 @@
         """Retrieve remote_filename and place in local_path"""
         if hasattr(self.backend, '_get'):
             self.backend._get(remote_filename, local_path)
+            local_path.setdata()
             if not local_path.exists():
                 raise BackendException(_("File %s not found locally after get "
                                          "from backend") % util.ufn(local_path.name))
-            local_path.setdata()
         else:
             raise NotImplementedError()
 

=== modified file 'duplicity/backends/webdavbackend.py'
--- duplicity/backends/webdavbackend.py	2014-05-07 21:41:31 +0000
+++ duplicity/backends/webdavbackend.py	2014-06-17 21:56:30 +0000
@@ -161,7 +161,7 @@
             and self.conn.host == self.parsed_url.hostname: return
 
         log.Info("WebDAV create connection on '%s'" % (self.parsed_url.hostname))
-        if self.conn: self.conn.close()
+        self._close()
         # http schemes needed for redirect urls from servers
         if self.parsed_url.scheme in ['webdav','http']:
             self.conn = httplib.HTTPConnection(self.parsed_url.hostname, self.parsed_url.port)
@@ -174,13 +174,15 @@
             raise FatalBackendException("WebDAV Unknown URI scheme: %s" % (self.parsed_url.scheme))
 
     def _close(self):
-        self.conn.close()
+        if self.conn:
+            self.conn.close()
 
     def request(self, method, path, data=None, redirected=0):
         """
         Wraps the connection.request method to retry once if authentication is
         required
         """
+        self._close() # or we get previous request's data or exception
         self.connect()
 
         quoted_path = urllib.quote(path,"/:~")
@@ -309,7 +311,6 @@
         for i in range(1,len(dirs)):
             d="/".join(dirs[0:i+1])+"/"
 
-            self._close() # or we get previous request's data or exception
             self.headers['Depth'] = "1"
             response = self.request("PROPFIND", d)
             del self.headers['Depth']
@@ -318,12 +319,10 @@
 
             if response.status == 404:
                 log.Info("Creating missing directory %s" % d)
-                self._close() # or we get previous request's data or exception
 
                 res = self.request("MKCOL", d)
                 if res.status != 201:
                     raise BackendException("WebDAV MKCOL %s failed: %s %s" % (d,res.status,res.reason))
-                self._close()
 
     def taste_href(self, href):
         """

=== modified file 'testing/manual/backendtest'
--- testing/manual/backendtest	2014-05-11 11:50:12 +0000
+++ testing/manual/backendtest	2014-06-17 21:56:30 +0000
@@ -52,6 +52,7 @@
     def setUp(self):
         super(ManualBackendBase, self).setUp()
         self.set_global('num_retries', 1)
+        self.set_global('ssl_no_check_certificate', True)
         self.setBackendInfo()
         if self.password is not None:
             self.set_environ("FTP_PASSWORD", self.password)