From: Brian Warner Date: Tue, 3 Jul 2007 20:18:14 +0000 (-0700) Subject: webish.py: handle errors during download better. Addresses #65. X-Git-Url: https://git.rkrishnan.org/components/com_hotproperty/reliability?a=commitdiff_plain;h=f15bb302a1e7dc6fda40ef02d63efc1ed0516452;p=tahoe-lafs%2Ftahoe-lafs.git 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 f29caa39..29ceaa79 100644 --- 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 508adf09..f2cb478d 100644 --- 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 e563eae3..0961a612 100644 --- 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 77e54f6b..e35c3ac1 100644 --- 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):