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).

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

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():

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

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):