From: Brian Warner <warner@allmydata.com>
Date: Tue, 3 Jul 2007 20:47:37 +0000 (-0700)
Subject: webish: improve reporting of web download errors that occur early enough
X-Git-Url: https://git.rkrishnan.org/components/com_hotproperty/%22doc.html/architecture.txt?a=commitdiff_plain;h=622acc690ac20ede473250600cb79d2df61cfbda;p=tahoe-lafs%2Ftahoe-lafs.git

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 29ceaa79..8b963fbb 100644
--- 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 f2cb478d..6582f994 100644
--- 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 0961a612..59b69c72 100644
--- 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 e35c3ac1..34d1942e 100644
--- 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