webish.py: handle errors during download better. Addresses #65.

Previously, exceptions during a web download caused a hang rather than some
kind of exception or error message. This patch improves the situation by
terminating the HTTP download rather than letting it hang forever. The
behavior still isn't ideal, however, because the error can occur too late to
abort the HTTP request cleanly (i.e. with an error code). In fact, the
Content-Type header and response code have already been set by the time any
download errors have been detected, so the browser is committed to displaying
an image or whatever (thus any error message we put into the stream is
unlikely to be displayed in a meaningful way).

diff --git a/src/allmydata/download.py b/src/allmydata/download.py
index f29caa3981337ed2db4c4b319426cd5946d297bb..29ceaa79fd72ad6c4e9489423c0aa027b9b2881f 100644 (file)
--- a/src/allmydata/download.py
+++ b/src/allmydata/download.py
@@ -73,6 +73,9 @@ class Output:
         # any memory usage beyond this.
         self.downloadable.write(plaintext)
 
+    def fail(self, why):
+        self.downloadable.fail(why)
+
     def close(self):
         self.crypttext_hash = self._crypttext_hasher.digest()
         self.plaintext_hash = self._plaintext_hasher.digest()
@@ -266,6 +269,7 @@ class FileDownloader:
         self._num_needed_shares = d['needed_shares']
 
         self._output = Output(downloadable, d['key'])
+        self._output.open()
 
         self.active_buckets = {} # k: shnum, v: bucket
         self._share_buckets = [] # list of (sharenum, bucket) tuples
@@ -294,6 +298,10 @@ class FileDownloader:
         d.addCallback(self._create_validated_buckets)
         # once we know that, we can download blocks from everybody
         d.addCallback(self._download_all_segments)
+        def _failed(why):
+            self._output.fail(why)
+            return why
+        d.addErrback(_failed)
         d.addCallback(self._done)
         return d
 
@@ -500,7 +508,6 @@ class FileDownloader:
         # of ValidatedBuckets, which themselves are wrappers around
         # RIBucketReader references.
         self.active_buckets = {} # k: shnum, v: ValidatedBucket instance
-        self._output.open()
 
         d = defer.succeed(None)
         for segnum in range(self._total_segments-1):
@@ -584,7 +591,7 @@ class FileName:
         self.f.write(data)
     def close(self):
         self.f.close()
-    def fail(self):
+    def fail(self, why):
         self.f.close()
         os.unlink(self._filename)
     def register_canceller(self, cb):
@@ -603,7 +610,7 @@ class Data:
     def close(self):
         self.data = "".join(self._data)
         del self._data
-    def fail(self):
+    def fail(self, why):
         del self._data
     def register_canceller(self, cb):
         pass # we won't use it
@@ -626,7 +633,7 @@ class FileHandle:
     def close(self):
         # the originator of the filehandle reserves the right to close it
         pass
-    def fail(self):
+    def fail(self, why):
         pass
     def register_canceller(self, cb):
         pass

diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py
index 508adf098b8e716c0281793f18610bcacc28dee2..f2cb478d702f2a3d77e038ed9e05546484294140 100644 (file)
--- a/src/allmydata/interfaces.py
+++ b/src/allmydata/interfaces.py
@@ -605,14 +605,15 @@ class IDecoder(Interface):
 
 class IDownloadTarget(Interface):
     def open():
-        """Called before any calls to write() or close()."""
+        """Called before any calls to write(), close(), or fail()."""
     def write(data):
         """Output some data to the target."""
     def close():
         """Inform the target that there is no more data to be written."""
-    def fail():
-        """fail() is called to indicate that the download has failed. No
-        further methods will be invoked on the IDownloadTarget after fail()."""
+    def fail(why):
+        """fail() is called to indicate that the download has failed. 'why'
+        is a Failure object indicating what went wrong. No further methods
+        will be invoked on the IDownloadTarget after fail()."""
     def register_canceller(cb):
         """The FileDownloader uses this to register a no-argument function
         that the target can call to cancel the download. Once this canceller
@@ -626,7 +627,10 @@ class IDownloadTarget(Interface):
 class IDownloader(Interface):
     def download(uri, target):
         """Perform a CHK download, sending the data to the given target.
-        'target' must provide IDownloadTarget."""
+        'target' must provide IDownloadTarget.
+
+        Returns a Deferred that fires (with the results of target.finish)
+        when the download is finished, or errbacks if something went wrong."""
 
 class IUploadable(Interface):
     def get_filehandle():

diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py
index e563eae34eee384041aff78061717bda9a88bf2a..0961a6129c525527d20b1dd5f2cb1c099057c2db 100644 (file)
--- a/src/allmydata/test/test_system.py
+++ b/src/allmydata/test/test_system.py
@@ -522,6 +522,15 @@ class SystemTest(testutil.SignalMixin, unittest.TestCase):
             self.failUnlessEqual(page, self.data)
         d.addCallback(_got_from_uri2)
 
+        # download from a bogus URI, make sure we get a reasonable error
+        def _get_from_bogus_uri(res):
+            return getPage(base + "download_uri/%s?filename=%s"
+                           % (self.mangle_uri(self.uri), "mydata567"))
+        d.addCallback(_get_from_bogus_uri)
+        def _got_from_bogus_uri(page):
+            self.failUnlessEqual(page, "problem during download\n")
+        d.addCallback(_got_from_bogus_uri)
+
         # download from a URI pasted into a form. Use POST, build a
         # multipart/form-data, submit it. This actualy redirects us to a
         # /download_uri?uri=%s URL, and twisted.web.client doesn't seem to

diff --git a/src/allmydata/webish.py b/src/allmydata/webish.py
index 77e54f6b532b6e2a42fe4b17ca153bdbbaba8662..e35c3ac14709f72f89f1e145117f4743f9e73b8a 100644 (file)
--- a/src/allmydata/webish.py
+++ b/src/allmydata/webish.py
@@ -330,7 +330,18 @@ class WebDownloadTarget:
         self._req.write(data)
     def close(self):
         self._req.finish()
-    def fail(self):
+    def fail(self, why):
+        # I think the content-type is already set, and the response code has
+        # already been sent, so we can't provide a clean error indication. We
+        # can emit text (which a browser might interpret as something else),
+        # and if we sent a Size header, they might notice that we've
+        # truncated the data. Keep the error message small to improve the
+        # chances of having our error response be shorter than the intended
+        # results.
+        #
+        # We don't have a lot of options, unfortunately.
+
+        self._req.write("problem during download\n")
         self._req.finish()
     def register_canceller(self, cb):
         pass
@@ -366,8 +377,11 @@ class Downloader(resource.Resource):
         req.setHeader("content-type", type)
         if encoding:
             req.setHeader('content-encoding', encoding)
+        # TODO: it would be nice to set the size header here
 
-        self._filenode.download(WebDownloadTarget(req))
+        d = self._filenode.download(WebDownloadTarget(req))
+        # exceptions during download are handled by the WebDownloadTarget
+        d.addErrback(lambda why: None)
         return server.NOT_DONE_YET
 
 class Manifest(rend.Page):