THRIFT-3600 Make TTwisted server send exception on unexpected handler error

Client: py

This closes #838
This closes #1424

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 23d4b7065..8851c3141 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<t_field*>& 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 886de44d2..02eb7f14f 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 @@ class TestHandler:
         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 @@ class ThriftTestCase(unittest.TestCase):
 
     @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 @@ class ThriftTestCase(unittest.TestCase):
 
         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)