From: Brian Warner Date: Thu, 25 Jun 2009 02:17:07 +0000 (-0700) Subject: Split out NoSharesError, stop adding attributes to NotEnoughSharesError, change human... X-Git-Tag: trac-4000~65 X-Git-Url: https://git.rkrishnan.org/specifications/banana.xhtml?a=commitdiff_plain;h=bd6ecc9f4436bdb736b392b14e73f2e9dbc75811;p=tahoe-lafs%2Ftahoe-lafs.git Split out NoSharesError, stop adding attributes to NotEnoughSharesError, change humanize_failure to include the original exception string, update tests, behave better if humanize_failure fails. --- diff --git a/src/allmydata/immutable/download.py b/src/allmydata/immutable/download.py index 9dfc0bb8..01ce4127 100644 --- a/src/allmydata/immutable/download.py +++ b/src/allmydata/immutable/download.py @@ -11,7 +11,7 @@ from allmydata import codec, hashtree, uri from allmydata.interfaces import IDownloadTarget, IDownloader, \ IFileURI, IVerifierURI, \ IDownloadStatus, IDownloadResults, IValidatedThingProxy, \ - IStorageBroker, NotEnoughSharesError, NoServersError, \ + IStorageBroker, NotEnoughSharesError, NoSharesError, NoServersError, \ UnableToFetchCriticalDownloadDataError from allmydata.immutable import layout from allmydata.monitor import Monitor @@ -818,9 +818,12 @@ class CiphertextDownloader(log.PrefixingLogMixin): self._results.timings["peer_selection"] = now - self._started if len(self._share_buckets) < self._verifycap.needed_shares: - raise NotEnoughSharesError("Failed to get enough shareholders", - len(self._share_buckets), - self._verifycap.needed_shares) + msg = "Failed to get enough shareholders: have %d, need %d" \ + % (len(self._share_buckets), self._verifycap.needed_shares) + if self._share_buckets: + raise NotEnoughSharesError(msg) + else: + raise NoSharesError(msg) #for s in self._share_vbuckets.values(): # for vb in s: @@ -906,8 +909,12 @@ class CiphertextDownloader(log.PrefixingLogMixin): potential_shnums = list(available_shnums - handled_shnums) if len(potential_shnums) < (self._verifycap.needed_shares - len(self.active_buckets)): have = len(potential_shnums) + len(self.active_buckets) - raise NotEnoughSharesError("Unable to activate enough shares", - have, self._verifycap.needed_shares) + msg = "Unable to activate enough shares: have %d, need %d" \ + % (have, self._verifycap.needed_shares) + if have: + raise NotEnoughSharesError(msg) + else: + raise NoSharesError(msg) # For the next share, choose a primary share if available, else a randomly chosen # secondary share. potential_shnums.sort() diff --git a/src/allmydata/immutable/encode.py b/src/allmydata/immutable/encode.py index 5c1f1869..5629a8b8 100644 --- a/src/allmydata/immutable/encode.py +++ b/src/allmydata/immutable/encode.py @@ -11,7 +11,7 @@ from allmydata.util import mathutil, hashutil, base32, log from allmydata.util.assertutil import _assert, precondition from allmydata.codec import CRSEncoder from allmydata.interfaces import IEncoder, IStorageBucketWriter, \ - IEncryptedUploadable, IUploadStatus, NotEnoughSharesError + IEncryptedUploadable, IUploadStatus, NotEnoughSharesError, NoSharesError """ The goal of the encoder is to turn the original file into a series of @@ -487,9 +487,12 @@ class Encoder(object): self.log("they weren't in our list of landlords", parent=ln, level=log.WEIRD, umid="TQGFRw") if len(self.landlords) < self.shares_of_happiness: - msg = "lost too many shareholders during upload: %s" % why - raise NotEnoughSharesError(msg, len(self.landlords), - self.shares_of_happiness) + msg = "lost too many shareholders during upload (still have %d, want %d): %s" % \ + (len(self.landlords), self.shares_of_happiness, why) + if self.landlords: + raise NotEnoughSharesError(msg) + else: + raise NoSharesError(msg) self.log("but we can still continue with %s shares, we'll be happy " "with at least %s" % (len(self.landlords), self.shares_of_happiness), @@ -504,7 +507,7 @@ class Encoder(object): # otherwise be consumed. Allow non-NotEnoughSharesError exceptions # to pass through as an unhandled errback. We use this in lieu of # consumeErrors=True to allow coding errors to be logged. - f.trap(NotEnoughSharesError) + f.trap(NotEnoughSharesError, NoSharesError) return None for d0 in dl: d0.addErrback(_eatNotEnoughSharesError) diff --git a/src/allmydata/immutable/upload.py b/src/allmydata/immutable/upload.py index 0dc541db..18f27677 100644 --- a/src/allmydata/immutable/upload.py +++ b/src/allmydata/immutable/upload.py @@ -17,7 +17,8 @@ from allmydata.util.assertutil import precondition from allmydata.util.rrefutil import add_version_to_remote_reference from allmydata.interfaces import IUploadable, IUploader, IUploadResults, \ IEncryptedUploadable, RIEncryptedUploadable, IUploadStatus, \ - NotEnoughSharesError, InsufficientVersionError, NoServersError + NotEnoughSharesError, NoSharesError, NoServersError, \ + InsufficientVersionError from allmydata.immutable import layout from pycryptopp.cipher.aes import AES @@ -286,11 +287,13 @@ class Tahoe2PeerSelector: placed_shares = self.total_shares - len(self.homeless_shares) if placed_shares < self.shares_of_happiness: msg = ("placed %d shares out of %d total (%d homeless), " + "want to place %d, " "sent %d queries to %d peers, " "%d queries placed some shares, %d placed none, " "got %d errors" % (self.total_shares - len(self.homeless_shares), self.total_shares, len(self.homeless_shares), + self.shares_of_happiness, self.query_count, self.num_peers_contacted, self.good_query_count, self.bad_query_count, self.error_count)) @@ -298,8 +301,10 @@ class Tahoe2PeerSelector: if self.last_failure_msg: msg += " (%s)" % (self.last_failure_msg,) log.msg(msg, level=log.UNUSUAL, parent=self._log_parent) - raise NotEnoughSharesError(msg, placed_shares, - self.shares_of_happiness) + if placed_shares: + raise NotEnoughSharesError(msg) + else: + raise NoSharesError(msg) else: # we placed enough to be happy, so we're done if self._status: diff --git a/src/allmydata/interfaces.py b/src/allmydata/interfaces.py index e130fa0f..fc820181 100644 --- a/src/allmydata/interfaces.py +++ b/src/allmydata/interfaces.py @@ -774,11 +774,11 @@ class IMutableFileNode(IFileNode, IMutableFilesystemNode): """ class NotEnoughSharesError(Exception): - def __init__(self, msg, got, needed): - Exception.__init__(self, msg) - self.got = got - self.needed = needed - self.servermap = None + """Download was unable to get enough shares, or upload was unable to + place 'shares_of_happiness' shares.""" + +class NoSharesError(Exception): + """Upload or Download was unable to get any shares at all.""" class UnableToFetchCriticalDownloadDataError(Exception): """I was unable to fetch some piece of critical data which is supposed to diff --git a/src/allmydata/mutable/retrieve.py b/src/allmydata/mutable/retrieve.py index 935ec50f..a72e1088 100644 --- a/src/allmydata/mutable/retrieve.py +++ b/src/allmydata/mutable/retrieve.py @@ -465,8 +465,7 @@ class Retrieve: self.log(format=format, level=log.WEIRD, umid="ezTfjw", **args) err = NotEnoughSharesError("%s, last failure: %s" % - (format % args, self._last_failure), - len(self.shares), k) + (format % args, self._last_failure)) if self._bad_shares: self.log("We found some bad shares this pass. You should " "update the servermap and try again to check " diff --git a/src/allmydata/test/test_cli.py b/src/allmydata/test/test_cli.py index 84d17334..f990ebb3 100644 --- a/src/allmydata/test/test_cli.py +++ b/src/allmydata/test/test_cli.py @@ -1425,8 +1425,8 @@ class Errors(GridTestMixin, CLITestMixin, unittest.TestCase): def _check1((rc, out, err)): self.failIfEqual(rc, 0) self.failUnless("410 Gone" in err, err) - self.failUnless("NotEnoughSharesError: 1 share found, but we need 3" in err, - err) + self.failUnlessIn("NotEnoughSharesError: ", err) + self.failUnlessIn("Failed to get enough shareholders: have 1, need 3", err) d.addCallback(_check1) return d diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index 9a7f4698..b9a1b593 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -16,7 +16,7 @@ from allmydata.util import idlib, mathutil from allmydata.util import log, base32 from allmydata.scripts import runner from allmydata.interfaces import IDirectoryNode, IFileNode, IFileURI, \ - NoSuchChildError, NotEnoughSharesError + NoSuchChildError, NotEnoughSharesError, NoSharesError from allmydata.monitor import Monitor from allmydata.mutable.common import NotMutableError from allmydata.mutable import layout as mutable_layout @@ -219,7 +219,7 @@ class SystemTest(SystemTestMixin, unittest.TestCase): bad_n = self.clients[1].create_node_from_uri(bad_u.to_string()) # this should cause an error during download - d = self.shouldFail2(NotEnoughSharesError, "'download bad node'", + d = self.shouldFail2(NoSharesError, "'download bad node'", None, bad_n.read, MemoryConsumer(), offset=2) return d @@ -234,12 +234,8 @@ class SystemTest(SystemTestMixin, unittest.TestCase): log.msg("finished downloading non-existend URI", level=log.UNUSUAL, facility="tahoe.tests") self.failUnless(isinstance(res, Failure)) - self.failUnless(res.check(NotEnoughSharesError), - "expected NotEnoughSharesError, got %s" % res) - # TODO: files that have zero peers should get a special kind - # of NotEnoughSharesError, which can be used to suggest that - # the URI might be wrong or that they've never uploaded the - # file in the first place. + self.failUnless(res.check(NoSharesError), + "expected NoSharesError, got %s" % res) d1.addBoth(_baduri_should_fail) return d1 d.addCallback(_download_nonexistent_uri) diff --git a/src/allmydata/test/test_upload.py b/src/allmydata/test/test_upload.py index ffeb4ee7..45c85d76 100644 --- a/src/allmydata/test/test_upload.py +++ b/src/allmydata/test/test_upload.py @@ -10,7 +10,8 @@ from foolscap.api import fireEventually import allmydata # for __full_version__ from allmydata import uri, monitor from allmydata.immutable import upload -from allmydata.interfaces import IFileURI, FileTooLargeError, NotEnoughSharesError +from allmydata.interfaces import IFileURI, FileTooLargeError, \ + NotEnoughSharesError, NoSharesError from allmydata.util.assertutil import precondition from allmydata.util.deferredutil import DeferredListShouldSucceed from no_network import GridTestMixin @@ -381,7 +382,7 @@ class FullServer(unittest.TestCase): self.u.parent = self.node def _should_fail(self, f): - self.failUnless(isinstance(f, Failure) and f.check(NotEnoughSharesError), f) + self.failUnless(isinstance(f, Failure) and f.check(NoSharesError), f) def test_data_large(self): data = DATA diff --git a/src/allmydata/test/test_web.py b/src/allmydata/test/test_web.py index 49c4e29f..c3bd3875 100644 --- a/src/allmydata/test/test_web.py +++ b/src/allmydata/test/test_web.py @@ -3237,16 +3237,17 @@ class Grid(GridTestMixin, WebErrorMixin, unittest.TestCase, ShouldFailMixin): d.addCallback(lambda ignored: self.shouldHTTPError("GET unrecoverable", - 410, "Gone", "NotEnoughSharesError", + 410, "Gone", "NoSharesError", self.GET, self.fileurls["0shares"])) def _check_zero_shares(body): self.failIf("" in body, body) body = " ".join(body.strip().split()) - exp = ("NotEnoughSharesError: no shares could be found. " + exp = ("NoSharesError: no shares could be found. " "Zero shares usually indicates a corrupt URI, or that " "no servers were connected, but it might also indicate " "severe corruption. You should perform a filecheck on " - "this object to learn more.") + "this object to learn more. The full error message is: " + "Failed to get enough shareholders: have 0, need 3") self.failUnlessEqual(exp, body) d.addCallback(_check_zero_shares) @@ -3258,12 +3259,12 @@ class Grid(GridTestMixin, WebErrorMixin, unittest.TestCase, ShouldFailMixin): def _check_one_share(body): self.failIf("" in body, body) body = " ".join(body.strip().split()) - exp = ("NotEnoughSharesError: 1 share found, but we need " - "3 to recover the file. This indicates that some " + exp = ("NotEnoughSharesError: This indicates that some " "servers were unavailable, or that shares have been " "lost to server departure, hard drive failure, or disk " "corruption. You should perform a filecheck on " - "this object to learn more.") + "this object to learn more. The full error message is:" + " Failed to get enough shareholders: have 1, need 3") self.failUnlessEqual(exp, body) d.addCallback(_check_one_share) diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 5c337581..28aec62c 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -1,11 +1,12 @@ from twisted.web import http, server +from twisted.python import log from zope.interface import Interface from nevow import loaders, appserver from nevow.inevow import IRequest from nevow.util import resource_filename from allmydata.interfaces import ExistingChildError, NoSuchChildError, \ - FileTooLargeError, NotEnoughSharesError + FileTooLargeError, NotEnoughSharesError, NoSharesError from allmydata.mutable.common import UnrecoverableFileError from allmydata.util import abbreviate # TODO: consolidate @@ -124,21 +125,20 @@ def humanize_failure(f): name = f.value.args[0] return ("No such child: %s" % name.encode("utf-8"), http.NOT_FOUND) if f.check(NotEnoughSharesError): - got = f.value.got - needed = f.value.needed - if got == 0: - t = ("NotEnoughSharesError: no shares could be found. " - "Zero shares usually indicates a corrupt URI, or that " - "no servers were connected, but it might also indicate " - "severe corruption. You should perform a filecheck on " - "this object to learn more.") - else: - t = ("NotEnoughSharesError: %d share%s found, but we need " - "%d to recover the file. This indicates that some " - "servers were unavailable, or that shares have been " - "lost to server departure, hard drive failure, or disk " - "corruption. You should perform a filecheck on " - "this object to learn more.") % (got, plural(got), needed) + t = ("NotEnoughSharesError: This indicates that some " + "servers were unavailable, or that shares have been " + "lost to server departure, hard drive failure, or disk " + "corruption. You should perform a filecheck on " + "this object to learn more.\n\nThe full error message is:\n" + "%s") % str(f.value) + return (t, http.GONE) + if f.check(NoSharesError): + t = ("NoSharesError: no shares could be found. " + "Zero shares usually indicates a corrupt URI, or that " + "no servers were connected, but it might also indicate " + "severe corruption. You should perform a filecheck on " + "this object to learn more.\n\nThe full error message is:\n" + "%s") % str(f.value) return (t, http.GONE) if f.check(UnrecoverableFileError): t = ("UnrecoverableFileError: the directory (or mutable file) could " @@ -170,7 +170,13 @@ class MyExceptionHandler(appserver.DefaultExceptionHandler): req.finishRequest(False) def renderHTTP_exception(self, ctx, f): - text, code = humanize_failure(f) + try: + text, code = humanize_failure(f) + except: + log.msg("exception in humanize_failure") + log.msg("argument was %s" % (f,)) + log.err() + text, code = str(f), None if code is not None: return self.simple(ctx, text, code) if f.check(server.UnsupportedMethod):