From b17c7cb86719167b026a069177edc17470ce4e5a Mon Sep 17 00:00:00 2001 From: Nobuaki Sukegawa Date: Wed, 10 Feb 2016 19:42:19 +0900 Subject: [PATCH] THRIFT-3600 Make TTwisted server send exception on unexpected handler error Client: py This closes #838 This closes #1424 --- .../cpp/src/thrift/generate/t_py_generator.cc | 112 ++++++++++-------- test/py.twisted/test_suite.py | 16 ++- 2 files changed, 75 insertions(+), 53 deletions(-) diff --git a/compiler/cpp/src/thrift/generate/t_py_generator.cc b/compiler/cpp/src/thrift/generate/t_py_generator.cc index 23d4b7065f0..8851c314133 100644 --- a/compiler/cpp/src/thrift/generate/t_py_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_py_generator.cc @@ -1898,8 +1898,6 @@ void t_py_generator::generate_process_function(t_service* tservice, t_function* } if (gen_twisted_) { - // TODO: Propagate arbitrary exception raised by handler to client as does plain "py" - // Generate the function call t_struct* arg_struct = tfunction->get_arglist(); const std::vector& fields = arg_struct->get_members(); @@ -1918,65 +1916,83 @@ void t_py_generator::generate_process_function(t_service* tservice, t_function* } f_service_ << ")" << endl; - // Shortcut out here for oneway functions if (tfunction->is_oneway()) { - f_service_ << indent() << "return d" << endl; - indent_down(); - f_service_ << endl; - return; - } - - f_service_ << indent() << "d.addCallback(self.write_results_success_" << tfunction->get_name() - << ", result, seqid, oprot)" << endl; - - if (xceptions.size() > 0) { - f_service_ << indent() << "d.addErrback(self.write_results_exception_" + f_service_ << indent() << "d.addErrback(self.handle_exception_" << tfunction->get_name() + << ", seqid)" << endl; + } else { + f_service_ << indent() << "d.addCallback(self.write_results_success_" << tfunction->get_name() + << ", result, seqid, oprot)" << endl + << indent() << "d.addErrback(self.write_results_exception_" << tfunction->get_name() << ", result, seqid, oprot)" << endl; } - - f_service_ << indent() << "return d" << endl; + f_service_ << indent() << "return d" << endl << endl; indent_down(); - f_service_ << endl; - indent(f_service_) << "def write_results_success_" << tfunction->get_name() - << "(self, success, result, seqid, oprot):" << endl; - indent_up(); - f_service_ << indent() << "result.success = success" << endl << indent() - << "oprot.writeMessageBegin(\"" << tfunction->get_name() - << "\", TMessageType.REPLY, seqid)" << endl << indent() << "result.write(oprot)" - << endl << indent() << "oprot.writeMessageEnd()" << endl << indent() - << "oprot.trans.flush()" << endl; - indent_down(); + if (tfunction->is_oneway()) { + indent(f_service_) << "def handle_exception_" << tfunction->get_name() + << "(self, error, seqid):" << endl; + } else { + indent(f_service_) << "def write_results_success_" << tfunction->get_name() + << "(self, success, result, seqid, oprot):" << endl; + indent_up(); + f_service_ << indent() << "result.success = success" << endl + << indent() << "oprot.writeMessageBegin(\"" << tfunction->get_name() + << "\", TMessageType.REPLY, seqid)" << endl + << indent() << "result.write(oprot)" << endl + << indent() << "oprot.writeMessageEnd()" << endl + << indent() << "oprot.trans.flush()" << endl + << endl; + indent_down(); - // Try block for a function with exceptions - if (!tfunction->is_oneway() && xceptions.size() > 0) { - f_service_ << endl; indent(f_service_) << "def write_results_exception_" << tfunction->get_name() << "(self, error, result, seqid, oprot):" << endl; - indent_up(); - f_service_ << indent() << "try:" << endl; + } + indent_up(); + if (!tfunction->is_oneway()) { + f_service_ << indent() << "msg_type = TMessageType.REPLY" << endl; + } + f_service_ << indent() << "try:" << endl; - // Kinda absurd - f_service_ << indent() << indent_str() << "error.raiseException()" << endl; + // Kinda absurd + f_service_ << indent() << indent_str() << "error.raiseException()" << endl; + if (!tfunction->is_oneway()) { for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) { - f_service_ << - indent() << "except " << type_name((*x_iter)->get_type()) << " as " << (*x_iter)->get_name() << ":" << endl; - if (!tfunction->is_oneway()) { - indent_up(); - f_service_ << indent() << "result." << (*x_iter)->get_name() << " = " - << (*x_iter)->get_name() << endl; - indent_down(); - } else { - f_service_ << indent() << "pass" << endl; - } + const string& xname = (*x_iter)->get_name(); + f_service_ << indent() << "except " << type_name((*x_iter)->get_type()) << " as " << xname + << ":" << endl; + indent_up(); + f_service_ << indent() << "result." << xname << " = " << xname << endl; + indent_down(); } - f_service_ << indent() << "oprot.writeMessageBegin(\"" << tfunction->get_name() - << "\", TMessageType.REPLY, seqid)" << endl << indent() << "result.write(oprot)" - << endl << indent() << "oprot.writeMessageEnd()" << endl << indent() - << "oprot.trans.flush()" << endl; - indent_down(); } + f_service_ << indent() << "except TTransport.TTransportException:" << endl + << indent() << indent_str() << "raise" << endl; + if (!tfunction->is_oneway()) { + f_service_ << indent() << "except TApplicationException as ex:" << endl + << indent() << indent_str() + << "logging.exception('TApplication exception in handler')" << endl + << indent() << indent_str() << "msg_type = TMessageType.EXCEPTION" << endl + << indent() << indent_str() << "result = ex" << endl + << indent() << "except Exception:" << endl + << indent() << indent_str() + << "logging.exception('Unexpected exception in handler')" << endl + << indent() << indent_str() << "msg_type = TMessageType.EXCEPTION" << endl + << indent() << indent_str() + << "result = TApplicationException(TApplicationException.INTERNAL_ERROR, " + "'Internal error')" + << endl + << indent() << "oprot.writeMessageBegin(\"" << tfunction->get_name() + << "\", msg_type, seqid)" << endl + << indent() << "result.write(oprot)" << endl + << indent() << "oprot.writeMessageEnd()" << endl + << indent() << "oprot.trans.flush()" << endl; + } else { + f_service_ << indent() << "except Exception:" << endl + << indent() << indent_str() + << "logging.exception('Exception in oneway handler')" << endl; + } + indent_down(); } else if (gen_tornado_) { // TODO: Propagate arbitrary exception raised by handler to client as does plain "py" diff --git a/test/py.twisted/test_suite.py b/test/py.twisted/test_suite.py index 886de44d26a..02eb7f14ff8 100755 --- a/test/py.twisted/test_suite.py +++ b/test/py.twisted/test_suite.py @@ -19,14 +19,17 @@ # under the License. # -import sys -import os import glob +import os +import sys import time + basepath = os.path.abspath(os.path.dirname(__file__)) sys.path.insert(0, os.path.join(basepath, 'gen-py.twisted')) sys.path.insert(0, glob.glob(os.path.join(basepath, '../../lib/py/build/lib.*'))[0]) +from thrift.Thrift import TApplicationException + from ThriftTest import ThriftTest from ThriftTest.ttypes import Xception, Xtruct from thrift.transport import TTwisted @@ -84,6 +87,7 @@ def testOneway(self, seconds): def fireOneway(t): self.onewaysQueue.put((t, time.time(), seconds)) reactor.callLater(seconds, fireOneway, time.time()) + raise Exception('') def testNest(self, thing): return thing @@ -171,7 +175,6 @@ def testStruct(self): @defer.inlineCallbacks def testException(self): - yield self.client.testException('Safe') try: yield self.client.testException('Xception') self.fail("should have gotten exception") @@ -181,12 +184,15 @@ def testException(self): try: yield self.client.testException("throw_undeclared") - self.fail("should have thrown exception") - except Exception: # type is undefined + self.fail("should have gotten exception") + except TApplicationException: pass + yield self.client.testException('Safe') + @defer.inlineCallbacks def testOneway(self): yield self.client.testOneway(1) start, end, seconds = yield self.handler.onewaysQueue.get() self.assertAlmostEquals(seconds, (end - start), places=1) + self.assertEquals((yield self.client.testI32(-1)), -1)