Fixed Python bug related to invalid return parameters, fixes #550

9 files changed, 94 insertions, 31 deletions

diff --git a/python/modules/IcePy/ObjectAdapter.cpp b/python/modules/IcePy/ObjectAdapter.cpp
index 03e6167182a..166ba647a53 100644
--- a/python/modules/IcePy/ObjectAdapter.cpp
+++ b/python/modules/IcePy/ObjectAdapter.cpp
@@ -17,6 +17,7 @@
 #include <Ice/ObjectAdapter.h>
 #include <Ice/Router.h>
 #include <Ice/ServantLocator.h>
+#include <Ice/Logger.h>
 
 #include <pythread.h>
 
@@ -177,7 +178,11 @@ IcePy::ServantLocatorWrapper::locate(const Ice::Current& current, Ice::LocalObje
     {
         if(PyTuple_GET_SIZE(res.get()) > 2)
         {
-            PyErr_WarnEx(PyExc_RuntimeWarning, STRCAST("invalid return value for ServantLocator::locate"), 1);
+            const Ice::CommunicatorPtr com = current.adapter->getCommunicator();
+            if(com->getProperties()->getPropertyAsIntWithDefault("Ice.Warn.Dispatch", 1) > 0)
+            {
+                com->getLogger()->warning("invalid return value for ServantLocator::locate");
+            }
             return 0;
         }
         servantObj = PyTuple_GET_ITEM(res.get(), 0);
@@ -196,7 +201,11 @@ IcePy::ServantLocatorWrapper::locate(const Ice::Current& current, Ice::LocalObje
     //
     if(!PyObject_IsInstance(servantObj, _objectType))
     {
-        PyErr_WarnEx(PyExc_RuntimeWarning, STRCAST("return value of ServantLocator::locate is not an Ice object"), 1);
+        const Ice::CommunicatorPtr com = current.adapter->getCommunicator();
+        if(com->getProperties()->getPropertyAsIntWithDefault("Ice.Warn.Dispatch", 1) > 0)
+        {
+            com->getLogger()->warning("return value of ServantLocator::locate is not an Ice object");
+        }
         return 0;
     }
 
diff --git a/python/modules/IcePy/Operation.cpp b/python/modules/IcePy/Operation.cpp
index d343d244c2a..0570640aca4 100644
--- a/python/modules/IcePy/Operation.cpp
+++ b/python/modules/IcePy/Operation.cpp
@@ -1371,9 +1371,7 @@ Operation::marshalResult(Ice::OutputStream& os, PyObject* result)
     {
         ostringstream ostr;
         ostr << "operation `" << fixIdent(name) << "' should return a tuple of length " << numResults;
-        string str = ostr.str();
-        PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
-        throw Ice::MarshalException(__FILE__, __LINE__);
+        throw Ice::MarshalException(__FILE__, __LINE__, ostr.str());
     }
 
     //
@@ -1407,12 +1405,18 @@ Operation::marshalResult(Ice::OutputStream& os, PyObject* result)
         PyObject* arg = PyTuple_GET_ITEM(t.get(), info->pos);
         if((!info->optional || arg != Unset) && !info->type->validate(arg))
         {
-            // TODO: Provide the parameter name instead?
-            ostringstream ostr;
-            ostr << "invalid value for out argument " << (info->pos + 1) << " in operation `" << dispatchName << "'";
-            string str = ostr.str();
-            PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
-            throw Ice::MarshalException(__FILE__, __LINE__);
+            try
+            {
+                PyException().raise();
+            }
+            catch(const Ice::UnknownException& ex)
+            {
+                // TODO: Provide the parameter name instead?
+                ostringstream ostr;
+                ostr << "invalid value for out argument " << (info->pos + 1) << " in operation `" << dispatchName;
+                ostr << "':\n" << ex.unknown;
+                throw Ice::MarshalException(__FILE__, __LINE__, ostr.str());
+            }
         }
     }
     if(returnType)
@@ -1420,11 +1424,16 @@ Operation::marshalResult(Ice::OutputStream& os, PyObject* result)
         PyObject* res = PyTuple_GET_ITEM(t.get(), 0);
         if((!returnType->optional || res != Unset) && !returnType->type->validate(res))
         {
-            ostringstream ostr;
-            ostr << "invalid return value for operation `" << dispatchName << "'";
-            string str = ostr.str();
-            PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
-            throw Ice::MarshalException(__FILE__, __LINE__);
+            try
+            {
+                PyException().raise();
+            }
+            catch(const Ice::UnknownException& ex)
+            {
+                ostringstream ostr;
+                ostr << "invalid return value for operation `" << dispatchName << "':\n" << ex.unknown;
+                throw Ice::MarshalException(__FILE__, __LINE__, ostr.str());
+            }
         }
     }
 
@@ -3777,7 +3786,6 @@ Upcall::dispatchImpl(PyObject* servant, const string& dispatchName, PyObject* ar
         ostr << "servant for identity " << communicator->identityToString(current.id)
              << " does not define operation `" << dispatchName << "'";
         string str = ostr.str();
-        PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
         Ice::UnknownException ex(__FILE__, __LINE__);
         ex.unknown = str;
         throw ex;
@@ -3793,7 +3801,6 @@ Upcall::dispatchImpl(PyObject* servant, const string& dispatchName, PyObject* ar
         ostr << "_iceDispatch method not found for identity " << communicator->identityToString(current.id)
              << " and operation `" << dispatchName << "'";
         string str = ostr.str();
-        PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
         Ice::UnknownException ex(__FILE__, __LINE__);
         ex.unknown = str;
         throw ex;
@@ -4108,9 +4115,7 @@ IcePy::BlobjectUpcall::response(PyObject* result)
         //
         if(!PyTuple_Check(result) || PyTuple_GET_SIZE(result) != 2)
         {
-            string str = "operation `ice_invoke' should return a tuple of length 2";
-            PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
-            throw Ice::MarshalException(__FILE__, __LINE__);
+            throw Ice::MarshalException(__FILE__, __LINE__, "operation `ice_invoke' should return a tuple of length 2");
         }
 
         PyObject* arg = PyTuple_GET_ITEM(result, 0);
@@ -4121,11 +4126,7 @@ IcePy::BlobjectUpcall::response(PyObject* result)
 #if PY_VERSION_HEX >= 0x03000000
         if(!PyBytes_Check(arg))
         {
-            ostringstream ostr;
-            ostr << "invalid return value for operation `ice_invoke'";
-            string str = ostr.str();
-            PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
-            throw Ice::MarshalException(__FILE__, __LINE__);
+            throw Ice::MarshalException(__FILE__, __LINE__, "invalid return value for operation `ice_invoke'");
         }
 
         Py_ssize_t sz = PyBytes_GET_SIZE(arg);
@@ -4139,11 +4140,7 @@ IcePy::BlobjectUpcall::response(PyObject* result)
 #else
         if(!PyBuffer_Check(arg))
         {
-            ostringstream ostr;
-            ostr << "invalid return value for operation `ice_invoke'";
-            string str = ostr.str();
-            PyErr_WarnEx(PyExc_RuntimeWarning, const_cast<char*>(str.c_str()), 1);
-            throw Ice::MarshalException(__FILE__, __LINE__);
+            throw Ice::MarshalException(__FILE__, __LINE__, "invalid return value for operation `ice_invoke'");
         }
 
         char* charBuf = 0;
diff --git a/python/test/Ice/exceptions/AllTests.py b/python/test/Ice/exceptions/AllTests.py
index 3aa59ab16f4..762058d61ef 100644
--- a/python/test/Ice/exceptions/AllTests.py
+++ b/python/test/Ice/exceptions/AllTests.py
@@ -813,6 +813,25 @@ def allTests(helper, communicator):
 
     print("ok")
 
+    sys.stdout.write("catching unknown non-Ice exception with futures... ")
+    sys.stdout.flush()
+    try:
+        try:
+            thrower.throwMarshalException(context={"response":""})
+        except Ice.UnknownLocalException as ex:
+            test("::Ice::MarshalException" in str(ex))
+        try:
+            thrower.throwMarshalException(context={"param":""})
+        except Ice.UnknownLocalException as ex:
+            test("::Ice::MarshalException" in str(ex))
+        try:
+            thrower.throwMarshalException()
+        except Ice.UnknownLocalException as ex:
+            test("::Ice::MarshalException" in str(ex))
+    except Ice.OperationNotExistException:
+        pass
+    print("ok")
+
     sys.stdout.write("catching exact types with AMI mapping... ")
     sys.stdout.flush()
 
diff --git a/python/test/Ice/exceptions/ServerAMD.py b/python/test/Ice/exceptions/ServerAMD.py
index 8bd9a6f35a8..b9418e7e29d 100755
--- a/python/test/Ice/exceptions/ServerAMD.py
+++ b/python/test/Ice/exceptions/ServerAMD.py
@@ -136,6 +136,12 @@ class ThrowerI(Test.Thrower):
         f.set_exception(Test.A())
         return f
 
+    def throwMarshalException(self, current):
+        if "return" in current.ctx:
+            return Ice.Future.completed(("", 0))
+        if "param" in current.ctx:
+            return Ice.Future.completed((0, ""))
+        return Ice.Future.completed(None)
 
 class ServerAMD(TestHelper):
 
diff --git a/python/test/Ice/exceptions/Test.ice b/python/test/Ice/exceptions/Test.ice
index adbcdcf3c9c..c9513cfa3ec 100644
--- a/python/test/Ice/exceptions/Test.ice
+++ b/python/test/Ice/exceptions/Test.ice
@@ -71,6 +71,8 @@ interface Thrower
 
     void throwAfterResponse();
     void throwAfterException() throws A;
+
+    int throwMarshalException(out int p);
 }
 
 interface WrongOperation
diff --git a/python/test/Ice/exceptions/TestI.py b/python/test/Ice/exceptions/TestI.py
index d4ea54c5df1..65860b6cad6 100644
--- a/python/test/Ice/exceptions/TestI.py
+++ b/python/test/Ice/exceptions/TestI.py
@@ -101,3 +101,9 @@ class ThrowerI(Test.Thrower):
         # Only relevant for AMD.
         #
         raise Test.A()
+
+    def throwMarshalException(self, current):
+        if "return" in current.ctx:
+            return ("", 0)
+        if "param" in current.ctx:
+            return (0, "")
diff --git a/python/test/Ice/servantLocator/AllTests.py b/python/test/Ice/servantLocator/AllTests.py
index 2aabd9103de..19bcf51e56f 100644
--- a/python/test/Ice/servantLocator/AllTests.py
+++ b/python/test/Ice/servantLocator/AllTests.py
@@ -217,4 +217,16 @@ def allTests(helper, communicator):
         test(False)
     print("ok")
 
+    sys.stdout.write("testing invalid locate return values ... ")
+    sys.stdout.flush()
+    try:
+        communicator.stringToProxy("invalidReturnValue:{0}".format(helper.getTestEndpoint())).ice_ping()
+    except Ice.ObjectNotExistException:
+        pass
+    try:
+        communicator.stringToProxy("invalidReturnType:{0}".format(helper.getTestEndpoint())).ice_ping()
+    except Ice.ObjectNotExistException:
+        pass
+    print ("ok")
+
     return obj
diff --git a/python/test/Ice/servantLocator/TestAMDI.py b/python/test/Ice/servantLocator/TestAMDI.py
index 0fc254af645..dae02342251 100644
--- a/python/test/Ice/servantLocator/TestAMDI.py
+++ b/python/test/Ice/servantLocator/TestAMDI.py
@@ -101,6 +101,12 @@ class ServantLocatorI(Ice.ServantLocator):
         if current.id.name == "unknown":
             return None
 
+        if current.id.name == "invalidReturnValue":
+            return (45, 12)
+
+        if current.id.name == "invalidReturnType":
+            return "invalid"
+
         test(current.id.name == "locate" or current.id.name == "finished")
         if current.id.name == "locate":
             self.exception(current)
diff --git a/python/test/Ice/servantLocator/TestI.py b/python/test/Ice/servantLocator/TestI.py
index 9e2345b0fe9..0ffe602ea49 100644
--- a/python/test/Ice/servantLocator/TestI.py
+++ b/python/test/Ice/servantLocator/TestI.py
@@ -89,6 +89,12 @@ class ServantLocatorI(Ice.ServantLocator):
         if current.id.name == "unknown":
             return None
 
+        if current.id.name == "invalidReturnValue":
+            return (45, 12)
+
+        if current.id.name == "invalidReturnType":
+            return "invalid"
+
         test(current.id.name == "locate" or current.id.name == "finished")
         if current.id.name == "locate":
             self.exception(current)