webish: improve reporting of web download errors that occur early enough

If the error occurs before any data has been sent, we can give a sensible
error message (code 500, stack trace, etc). This will cover most of the error
cases. The ones that aren't covered are when we run out of good peers after
successfully decoding the first segment, either because they go away or
because their shares are corrupt.

src/allmydata/download.py

@@ -35,14 +35,12 @@ class Output:
         self._segment_number = 0
         self._plaintext_hash_tree = None
         self._crypttext_hash_tree = None
+        self._opened = False
 
     def setup_hashtrees(self, plaintext_hashtree, crypttext_hashtree):
         self._plaintext_hash_tree = plaintext_hashtree
         self._crypttext_hash_tree = crypttext_hashtree
 
-    def open(self):
-        self.downloadable.open()
-
     def write_segment(self, crypttext):
         self.length += len(crypttext)
 
@@ -71,9 +69,13 @@ class Output:
         self._segment_number += 1
         # We're still at 1*segment_size. The Downloadable is responsible for
         # any memory usage beyond this.
+        if not self._opened:
+            self._opened = True
+            self.downloadable.open()
         self.downloadable.write(plaintext)
 
     def fail(self, why):
+        log.msg("UNUSUAL: download failed: %s" % why)
         self.downloadable.fail(why)
 
     def close(self):
@@ -269,7 +271,6 @@ 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

src/allmydata/interfaces.py

@@ -605,7 +605,9 @@ class IDecoder(Interface):
 
 class IDownloadTarget(Interface):
     def open():
-        """Called before any calls to write(), close(), or fail()."""
+        """Called before any calls to write() or close(). If an error
+        occurs before any data is available, fail() may be called without
+        a previous call to open()."""
     def write(data):
         """Output some data to the target."""
     def close():

src/allmydata/test/test_system.py

@@ -14,7 +14,7 @@ from foolscap.eventual import flushEventualQueue
 from twisted.python import log
 from twisted.python.failure import Failure
 from twisted.web.client import getPage
-from twisted.web.error import PageRedirect
+from twisted.web.error import PageRedirect, Error
 
 def flush_but_dont_ignore(res):
     d = flushEventualQueue()
@@ -527,9 +527,12 @@ class SystemTest(testutil.SignalMixin, unittest.TestCase):
             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):
+        d.addBoth(self.shouldFail, Error, "downloading bogus URI",
-            self.failUnlessEqual(page, "problem during download\n")
+                  "NotEnoughPeersError")
-        d.addCallback(_got_from_bogus_uri)
+
+        # TODO: mangle the second segment of a file, to test errors that
+        # occur after we've already sent some good data, which uses a
+        # different error path.
 
         # download from a URI pasted into a form. Use POST, build a
         # multipart/form-data, submit it. This actualy redirects us to a

src/allmydata/webish.py

@@ -1,6 +1,6 @@
 
 from twisted.application import service, strports
-from twisted.web import static, resource, server, html
+from twisted.web import static, resource, server, html, http
 from twisted.python import util, log
 from nevow import inevow, rend, loaders, appserver, url, tags as T
 from nevow.static import File as nevow_File # TODO: merge with static.File?
@@ -322,27 +322,47 @@ class Directory(rend.Page):
 
 class WebDownloadTarget:
     implements(IDownloadTarget)
-    def __init__(self, req):
+    def __init__(self, req, content_type, content_encoding):
         self._req = req
+        self._content_type = content_type
+        self._content_encoding = content_encoding
+        self._opened = False
+
     def open(self):
-        pass
+        self._opened = True
+        self._req.setHeader("content-type", self._content_type)
+        if self._content_encoding:
+            self._req.setHeader('content-encoding', self._content_encoding)
+        # TODO: it would be nice to set the size header here
+
     def write(self, data):
         self._req.write(data)
     def close(self):
         self._req.finish()
+
     def fail(self, why):
-        # I think the content-type is already set, and the response code has
+        if self._opened:
-        # already been sent, so we can't provide a clean error indication. We
+            # The content-type is already set, and the response code
-        # can emit text (which a browser might interpret as something else),
+            # has already been sent, so we can't provide a clean error
-        # and if we sent a Size header, they might notice that we've
+            # indication. We can emit text (which a browser might interpret
-        # truncated the data. Keep the error message small to improve the
+            # as something else), and if we sent a Size header, they might
-        # chances of having our error response be shorter than the intended
+            # notice that we've truncated the data. Keep the error message
-        # results.
+            # 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")
+        else:
+            # We haven't written anything yet, so we can provide a sensible
+            # error message.
+            msg = str(why.type)
+            msg.replace("\n", "|")
+            self._req.setResponseCode(http.INTERNAL_SERVER_ERROR, msg)
+            self._req.setHeader("content-type", "text/plain")
+            # TODO: HTML-formatted exception?
+            self._req.write(str(why))
         self._req.finish()
+
     def register_canceller(self, cb):
         pass
     def finish(self):
@@ -374,12 +394,8 @@ class Downloader(resource.Resource):
                              static.File.contentTypes,
                              static.File.contentEncodings,
                              defaultType="text/plain")
-        req.setHeader("content-type", type)
-        if encoding:
-            req.setHeader('content-encoding', encoding)
-        # TODO: it would be nice to set the size header here
 
-        d = self._filenode.download(WebDownloadTarget(req))
+        d = self._filenode.download(WebDownloadTarget(req, type, encoding))
         # exceptions during download are handled by the WebDownloadTarget
         d.addErrback(lambda why: None)
         return server.NOT_DONE_YET