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

19 files changed, 146 insertions, 31 deletions

diff --git a/cpp/test/Ice/servantLocator/ServantLocatorI.cpp b/cpp/test/Ice/servantLocator/ServantLocatorI.cpp
index 2282aaca474..752b55ad044 100644
--- a/cpp/test/Ice/servantLocator/ServantLocatorI.cpp
+++ b/cpp/test/Ice/servantLocator/ServantLocatorI.cpp
@@ -43,6 +43,11 @@ ServantLocatorI::locate(const Ice::Current& current, Ice::LocalObjectPtr& cookie
         return 0;
     }
 
+    if(current.id.name == "invalidReturnValue" || current.id.name == "invalidReturnType")
+    {
+        return 0;
+    }
+
     test(current.id.name == "locate" || current.id.name == "finished");
     if(current.id.name == "locate")
     {
diff --git a/csharp/test/Ice/servantLocator/ServantLocatorAMDI.cs b/csharp/test/Ice/servantLocator/ServantLocatorAMDI.cs
index d67230a694d..e56c84f7525 100644
--- a/csharp/test/Ice/servantLocator/ServantLocatorAMDI.cs
+++ b/csharp/test/Ice/servantLocator/ServantLocatorAMDI.cs
@@ -50,6 +50,12 @@ namespace Ice
                         return null;
                     }
 
+                    if(current.id.name.Equals("invalidReturnValue") || current.id.name.Equals("invalidReturnType"))
+                    {
+                        cookie = null;
+                        return null;
+                    }
+
                     test(current.id.name.Equals("locate") || current.id.name.Equals("finished"));
                     if(current.id.name.Equals("locate"))
                     {
diff --git a/csharp/test/Ice/servantLocator/ServantLocatorI.cs b/csharp/test/Ice/servantLocator/ServantLocatorI.cs
index 4cdc5b96b2b..6e0708546ff 100644
--- a/csharp/test/Ice/servantLocator/ServantLocatorI.cs
+++ b/csharp/test/Ice/servantLocator/ServantLocatorI.cs
@@ -48,6 +48,12 @@ namespace Ice
                     return null;
                 }
 
+                if(current.id.name.Equals("invalidReturnValue") || current.id.name.Equals("invalidReturnType"))
+                {
+                    cookie = null;
+                    return null;
+                }
+
                 test(current.id.name.Equals("locate") || current.id.name.Equals("finished"));
                 if(current.id.name.Equals("locate"))
                 {
diff --git a/java-compat/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java b/java-compat/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java
index accd91b9fa7..0f131ffffee 100644
--- a/java-compat/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java
+++ b/java-compat/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java
@@ -57,6 +57,11 @@ public final class AMDServantLocatorI implements Ice.ServantLocator
             return null;
         }
 
+        if(current.id.name.equals("invalidReturnValue") || current.id.name.equals("invalidReturnType"))
+        {
+            return null;
+        }
+
         test(current.id.name.equals("locate") || current.id.name.equals("finished"));
         if(current.id.name.equals("locate"))
         {
diff --git a/java-compat/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java b/java-compat/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java
index 8dfb7665841..7ac1f491cdb 100644
--- a/java-compat/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java
+++ b/java-compat/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java
@@ -57,6 +57,11 @@ public final class ServantLocatorI implements Ice.ServantLocator
             return null;
         }
 
+        if(current.id.name.equals("invalidReturnValue") || current.id.name.equals("invalidReturnType"))
+        {
+            return null;
+        }
+
         test(current.id.name.equals("locate") || current.id.name.equals("finished"));
         if(current.id.name.equals("locate"))
         {
diff --git a/java/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java b/java/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java
index 4509e1da798..15496d61798 100644
--- a/java/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java
+++ b/java/test/src/main/java/test/Ice/servantLocator/AMDServantLocatorI.java
@@ -57,6 +57,11 @@ public final class AMDServantLocatorI implements ServantLocator
             return new ServantLocator.LocateResult();
         }
 
+        if(current.id.name.equals("invalidReturnValue") || current.id.name.equals("invalidReturnType"))
+        {
+            return new ServantLocator.LocateResult();
+        }
+
         test(current.id.name.equals("locate") || current.id.name.equals("finished"));
         if(current.id.name.equals("locate"))
         {
diff --git a/java/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java b/java/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java
index 8217d99e124..ac488ff2681 100644
--- a/java/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java
+++ b/java/test/src/main/java/test/Ice/servantLocator/ServantLocatorI.java
@@ -57,6 +57,11 @@ public final class ServantLocatorI implements ServantLocator
             return new ServantLocator.LocateResult();
         }
 
+        if(current.id.name.equals("invalidReturnValue") || current.id.name.equals("invalidReturnType"))
+        {
+            return new ServantLocator.LocateResult();
+        }
+
         test(current.id.name.equals("locate") || current.id.name.equals("finished"));
         if(current.id.name.equals("locate"))
         {
diff --git a/js/test/Ice/servantLocator/ServantLocatorI.js b/js/test/Ice/servantLocator/ServantLocatorI.js
index c2267194751..55ea4529846 100644
--- a/js/test/Ice/servantLocator/ServantLocatorI.js
+++ b/js/test/Ice/servantLocator/ServantLocatorI.js
@@ -41,6 +41,11 @@
                 return null;
             }
 
+            if(current.id.name == "invalidReturnValue" || current.id.name == "invalidReturnType")
+            {
+                return null;
+            }
+
             test(current.id.name == "locate" || current.id.name == "finished");
             if(current.id.name == "locate")
             {
diff --git a/objective-c/test/Ice/servantLocator/ServantLocatorI.m b/objective-c/test/Ice/servantLocator/ServantLocatorI.m
index fd94a5c6411..ca028a5a18e 100644
--- a/objective-c/test/Ice/servantLocator/ServantLocatorI.m
+++ b/objective-c/test/Ice/servantLocator/ServantLocatorI.m
@@ -36,6 +36,11 @@
         return 0;
     }
 
+    if([current.id_.name isEqual:@"invalidReturnValue"] || [current.id_.name isEqual:@"invalidReturnType"])
+    {
+        return 0;
+    }
+
     test([current.id_.name isEqual:@"locate"] || [current.id_.name isEqual:@"finished"]);
     if([current.id_.name isEqual:@"locate"])
     {
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)
diff --git a/swift/test/Ice/servantLocator/ServantLocatorI.swift b/swift/test/Ice/servantLocator/ServantLocatorI.swift
index a4ed8c5ee9f..99e7220620c 100644
--- a/swift/test/Ice/servantLocator/ServantLocatorI.swift
+++ b/swift/test/Ice/servantLocator/ServantLocatorI.swift
@@ -43,6 +43,11 @@ class ServantLocatorI: Ice.ServantLocator {
             return (nil, nil)
         }
 
+        if(curr.id.name == "invalidReturnValue" || curr.id.name == "invalidReturnType") {
+            return (nil, nil)
+        }
+
+
         try _helper.test(curr.id.name == "locate" || curr.id.name == "finished")
 
         if curr.id.name == "locate" {