THRIFT-3600 Make TTwisted server send exception on unexpected handler error
Client: py
This closes #838
This closes #1424
diff --git a/compiler/cpp/src/thrift/generate/t_py_generator.cc b/compiler/cpp/src/thrift/generate/t_py_generator.cc
index 23d4b70..8851c31 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 @@
}
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<t_field*>& fields = arg_struct->get_members();
@@ -1918,65 +1916,83 @@
}
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;
-
- // Kinda absurd
- f_service_ << indent() << indent_str() << "error.raiseException()" << endl;
- 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;
- }
- }
- 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();
}
+ 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;
+ if (!tfunction->is_oneway()) {
+ for (x_iter = xceptions.begin(); x_iter != xceptions.end(); ++x_iter) {
+ 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() << "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 886de44..02eb7f1 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 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 @@
@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 @@
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)