From 69c7c405107d19ac8c7b7430fcada1a7801e299c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Thu, 15 Oct 2020 11:43:42 -0400 Subject: [PATCH] handle Deferred from render --- src/allmydata/test/web/test_grid.py | 5 + src/allmydata/web/common.py | 198 +++++++++++++++++++++------- src/allmydata/web/directory.py | 10 +- src/allmydata/web/filenode.py | 43 ++---- src/allmydata/web/operations.py | 20 +-- src/allmydata/web/unlinked.py | 5 +- 6 files changed, 188 insertions(+), 93 deletions(-) diff --git a/src/allmydata/test/web/test_grid.py b/src/allmydata/test/web/test_grid.py index 01eb93fa7..8f61781d4 100644 --- a/src/allmydata/test/web/test_grid.py +++ b/src/allmydata/test/web/test_grid.py @@ -18,6 +18,10 @@ from allmydata.storage.shares import get_share_file from allmydata.scripts.debug import CorruptShareOptions, corrupt_share from allmydata.immutable import upload from allmydata.mutable import publish + +from ...web.common import ( + render_exception, +) from .. import common_util as testutil from ..common import WebErrorMixin, ShouldFailMixin from ..no_network import GridTestMixin @@ -34,6 +38,7 @@ class CompletelyUnhandledError(Exception): pass class ErrorBoom(object, resource.Resource): + @render_exception def render(self, req): raise CompletelyUnhandledError("whoops") diff --git a/src/allmydata/web/common.py b/src/allmydata/web/common.py index 102e67adc..9c88d832a 100644 --- a/src/allmydata/web/common.py +++ b/src/allmydata/web/common.py @@ -3,15 +3,43 @@ import time import json from functools import wraps +from hyperlink import ( + DecodedURL, +) + from twisted.web import ( http, resource, server, template, ) +from twisted.web.template import ( + tags, +) +from twisted.web.util import ( + FailureElement, + redirectTo, +) +from twisted.python.reflect import ( + fullyQualifiedName, +) +from twisted.python.urlpath import ( + URLPath, +) from twisted.python import log +from twisted.python.failure import ( + Failure, +) +from twisted.internet.defer import ( + Deferred, + maybeDeferred, +) +from twisted.web.resource import ( + IResource, +) from nevow import appserver from nevow.inevow import IRequest + from allmydata import blacklist from allmydata.interfaces import ( EmptyPathnameComponentError, @@ -320,48 +348,10 @@ def humanize_failure(f): class MyExceptionHandler(appserver.DefaultExceptionHandler, object): - def simple(self, ctx, text, code=http.BAD_REQUEST): - req = IRequest(ctx) - req.setResponseCode(code) - #req.responseHeaders.setRawHeaders("content-encoding", []) - #req.responseHeaders.setRawHeaders("content-disposition", []) - req.setHeader("content-type", "text/plain;charset=utf-8") - if isinstance(text, unicode): - text = text.encode("utf-8") - req.setHeader("content-length", b"%d" % len(text)) - req.write(text) - # TODO: consider putting the requested URL here - req.finishRequest(False) - def renderHTTP_exception(self, ctx, 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): - # twisted.web.server.Request.render() has support for transforming - # this into an appropriate 501 NOT_IMPLEMENTED or 405 NOT_ALLOWED - # return code, but nevow does not. - req = IRequest(ctx) - method = req.method - return self.simple(ctx, - "I don't know how to treat a %s request." % method, - http.NOT_IMPLEMENTED) req = IRequest(ctx) - accept = req.getHeader("accept") - if not accept: - accept = "*/*" - if "*/*" in accept or "text/*" in accept or "text/html" in accept: - super = appserver.DefaultExceptionHandler - return super.renderHTTP_exception(self, ctx, f) - # use plain text - traceback = f.getTraceback() - return self.simple(ctx, traceback, http.INTERNAL_SERVER_ERROR) + req.write(_renderHTTP_exception(req, f)) + req.finishRequest(False) class NeedOperationHandleError(WebError): @@ -481,16 +471,130 @@ def exception_to_child(f): return g -def render_exception(f): +def render_exception(render): """ Decorate a ``render_*`` method with exception handling behavior to render an error page reflecting the exception. """ - @wraps(f) + @wraps(render) def g(self, request): - try: - return f(self, request) - except Exception as e: - description, status = humanize_exception(e) - return resource.ErrorPage(status, "Error", description).render(request) + bound_render = render.__get__(self, type(self)) + result = maybeDeferred(bound_render, request) + if isinstance(result, Deferred): + result.addBoth(_finish, bound_render, request) + return server.NOT_DONE_YET + elif isinstance(result, bytes): + return result + elif result == server.NOT_DONE_YET: + return server.NOT_DONE_YET + else: + raise Exception("{!r} returned unusable {!r}".format( + fullyQualifiedName(bound_render), + result, + )) + return g + + +def _finish(result, render, request): + if isinstance(result, Failure): + _finish( + _renderHTTP_exception(request, result), + render, + request, + ) + elif IResource.providedBy(result): + # If result is also using @render_exception then we don't want to + # double-apply the logic. This leads to an attempt to double-finish + # the request. If it isn't using @render_exception then you should + # fix it so it is. + result.render(request) + elif isinstance(result, bytes): + request.write(result) + request.finish() + elif isinstance(result, URLPath): + if result.netloc == b"": + root = URLPath.fromRequest(request) + result.scheme = root.scheme + result.netloc = root.netloc + _finish(redirectTo(str(result), request), render, request) + elif isinstance(result, DecodedURL): + _finish(redirectTo(str(result), request), render, request) + elif result is None: + request.finish() + elif result == server.NOT_DONE_YET: + pass + else: + log.err("Request for {!r} handled by {!r} returned unusable {!r}".format( + request.uri, + fullyQualifiedName(render), + result, + )) + request.setResponseCode(http.INTERNAL_SERVER_ERROR) + _finish(b"", render, request) + + +def _renderHTTP_exception(request, failure): + try: + text, code = humanize_failure(failure) + except: + log.msg("exception in humanize_failure") + log.msg("argument was %s" % (failure,)) + log.err() + text = str(failure) + code = None + + if code is not None: + return _renderHTTP_exception_simple(request, text, code) + + if failure.check(server.UnsupportedMethod): + # twisted.web.server.Request.render() has support for transforming + # this into an appropriate 501 NOT_IMPLEMENTED or 405 NOT_ALLOWED + # return code, but nevow does not. + method = request.method + return _renderHTTP_exception_simple( + request, + "I don't know how to treat a %s request." % (method,), + http.NOT_IMPLEMENTED, + ) + + accept = request.getHeader("accept") + if not accept: + accept = "*/*" + if "*/*" in accept or "text/*" in accept or "text/html" in accept: + request.setResponseCode(http.INTERNAL_SERVER_ERROR) + return template.renderElement( + request, + tags.html( + tags.head( + tags.title(u"Exception"), + ), + tags.body( + FailureElement(failure), + ), + ), + ) + + # use plain text + traceback = failure.getTraceback() + return _renderHTTP_exception_simple( + request, + traceback, + http.INTERNAL_SERVER_ERROR, + ) + + +def _renderHTTP_exception_simple(request, text, code): + request.setResponseCode(code) + request.setHeader("content-type", "text/plain;charset=utf-8") + if isinstance(text, unicode): + text = text.encode("utf-8") + request.setHeader("content-length", b"%d" % len(text)) + return text + + +def handle_when_done(req, d): + when_done = get_arg(req, "when_done", None) + if when_done: + d.addCallback(lambda res: DecodedURL.from_text(when_done.decode("utf-8"))) + return d diff --git a/src/allmydata/web/directory.py b/src/allmydata/web/directory.py index 4d7aa1bd1..39bc297f5 100644 --- a/src/allmydata/web/directory.py +++ b/src/allmydata/web/directory.py @@ -58,6 +58,7 @@ from allmydata.web.common import ( SlotsSequenceElement, exception_to_child, render_exception, + handle_when_done, ) from allmydata.web.filenode import ReplaceMeMixin, \ FileNodeHandler, PlaceHolderNodeHandler @@ -206,6 +207,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): ) return make_handler_for(node, self.client, self.node, name) + @render_exception def render_DELETE(self, req): assert self.parentnode and self.name d = self.parentnode.delete(self.name) @@ -310,13 +312,7 @@ class DirectoryNodeHandler(ReplaceMeMixin, Resource, object): else: raise WebError("POST to a directory with bad t=%s" % t) - when_done = get_arg(req, "when_done", None) - if when_done: - def done(res): - req.redirect(when_done) - return res - d.addCallback(done) - return d + return handle_when_done(req, d) def _POST_mkdir(self, req): name = get_arg(req, "name", "") diff --git a/src/allmydata/web/filenode.py b/src/allmydata/web/filenode.py index 0ecc8cc52..b36ce0af8 100644 --- a/src/allmydata/web/filenode.py +++ b/src/allmydata/web/filenode.py @@ -1,6 +1,10 @@ import json +from hyperlink import ( + DecodedURL, +) + from twisted.web import http, static from twisted.internet import defer from twisted.web.resource import ( @@ -8,8 +12,6 @@ from twisted.web.resource import ( ErrorPage, ) -from nevow import url - from allmydata.interfaces import ExistingChildError from allmydata.monitor import Monitor from allmydata.immutable.upload import FileHandle @@ -34,8 +36,8 @@ from allmydata.web.common import ( render_exception, should_create_intermediate_directories, text_plain, - MyExceptionHandler, WebError, + handle_when_done, ) from allmydata.web.check_results import ( CheckResultsRenderer, @@ -150,10 +152,7 @@ class PlaceHolderNodeHandler(Resource, ReplaceMeMixin): # placeholder. raise WebError("POST to a file: bad t=%s" % t) - when_done = get_arg(req, "when_done", None) - if when_done: - d.addCallback(lambda res: when_done) - return d + return handle_when_done(req, d) class FileNodeHandler(Resource, ReplaceMeMixin, object): @@ -315,10 +314,7 @@ class FileNodeHandler(Resource, ReplaceMeMixin, object): else: raise WebError("POST to file: bad t=%s" % t) - when_done = get_arg(req, "when_done", None) - if when_done: - d.addCallback(lambda res: url.URL.fromString(when_done)) - return d + return handle_when_done(req, d) def _maybe_literal(self, res, Results_Class): if res: @@ -485,23 +481,11 @@ class FileDownloader(Resource, object): if req.method == "HEAD": return "" - finished = [] - def _request_finished(ign): - finished.append(True) - req.notifyFinish().addBoth(_request_finished) - d = self.filenode.read(req, first, size) - def _finished(ign): - if not finished: - req.finish() def _error(f): lp = log.msg("error during GET", facility="tahoe.webish", failure=f, level=log.UNUSUAL, umid="xSiF3w") - if finished: - log.msg("but it's too late to tell them", parent=lp, - level=log.UNUSUAL, umid="j1xIbw") - return req._tahoe_request_had_error = f # for HTTP-style logging if req.startedWriting: # The content-type is already set, and the response code has @@ -513,15 +497,16 @@ class FileDownloader(Resource, object): # error response be shorter than the intended results. # # We don't have a lot of options, unfortunately. - req.write("problem during download\n") - req.finish() + return b"problem during download\n" else: # We haven't written anything yet, so we can provide a # sensible error message. - eh = MyExceptionHandler() - eh.renderHTTP_exception(req, f) - d.addCallbacks(_finished, _error) - return req.deferred + return f + d.addCallbacks( + lambda ignored: None, + _error, + ) + return d def _file_json_metadata(req, filenode, edge_metadata): diff --git a/src/allmydata/web/operations.py b/src/allmydata/web/operations.py index 21c2ec7ef..25e267038 100644 --- a/src/allmydata/web/operations.py +++ b/src/allmydata/web/operations.py @@ -1,10 +1,15 @@ import time -from nevow import url +from hyperlink import ( + DecodedURL, +) from twisted.web.template import ( renderer, tags as T, ) +from twisted.python.urlpath import ( + URLPath, +) from twisted.python.failure import Failure from twisted.internet import reactor, defer from twisted.web import resource @@ -84,17 +89,14 @@ class OphandleTable(resource.Resource, service.Service): """ :param allmydata.webish.MyRequest req: """ - ophandle = get_arg(req, "ophandle") + ophandle = get_arg(req, "ophandle").decode("utf-8") assert ophandle - target = get_root(req) + "/operations/" + ophandle + root = DecodedURL.from_text(unicode(URLPath.fromRequest(req))) + target = root.click(u"/").child(u"operations", ophandle) output = get_arg(req, "output") if output: - target = target + "?output=%s" % output - - # XXX: We have to use nevow.url here because nevow.appserver - # is unhappy with anything else; so this gets its own ticket. - # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3314 - return url.URL.fromString(target) + target = target.add(u"output", output.decode("utf-8")) + return target @exception_to_child def getChild(self, name, req): diff --git a/src/allmydata/web/unlinked.py b/src/allmydata/web/unlinked.py index f94e32240..f63006e7e 100644 --- a/src/allmydata/web/unlinked.py +++ b/src/allmydata/web/unlinked.py @@ -2,6 +2,9 @@ import urllib from twisted.web import http from twisted.internet import defer +from twisted.python.urlpath import ( + URLPath, +) from twisted.python.filepath import FilePath from twisted.web.resource import Resource from twisted.web.template import ( @@ -66,7 +69,7 @@ def POSTUnlinkedCHK(req, client): def _done(upload_results, redir_to): if "%(uri)s" in redir_to: redir_to = redir_to.replace("%(uri)s", urllib.quote(upload_results.get_uri())) - return url.URL.fromString(redir_to) + return URLPath.fromString(redir_to) d.addCallback(_done, when_done) else: # return the Upload Results page, which includes the URI