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.

diff --git a/src/allmydata/download.py b/src/allmydata/download.py
index 29ceaa79fd72ad6c4e9489423c0aa027b9b2881f..8b963fbb655568482a5310f181ead69e30850a57 100644 (file)
--- a/src/allmydata/download.py
+++ b/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

diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py
index f2cb478d702f2a3d77e038ed9e05546484294140..6582f99463f60f25fe88684c8ca4185015b350c4 100644 (file)
--- a/src/allmydata/interfaces.py
+++ b/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():

diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py
index 0961a6129c525527d20b1dd5f2cb1c099057c2db..59b69c728bc64c125dba019d89da28a9621095bd 100644 (file)
--- a/src/allmydata/test/test_system.py
+++ b/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):
-            self.failUnlessEqual(page, "problem during download\n")
-        d.addCallback(_got_from_bogus_uri)
+        d.addBoth(self.shouldFail, Error, "downloading bogus URI",
+                  "NotEnoughPeersError")
+
+        # 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

diff --git a/src/allmydata/webish.py b/src/allmydata/webish.py
index e35c3ac14709f72f89f1e145117f4743f9e73b8a..34d1942e4b0067fa87fb07c4dbbb93cdc591cd54 100644 (file)
--- a/src/allmydata/webish.py
+++ b/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
-        # 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")
+        if self._opened:
+            # 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")
+        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