Fixed potential deadlock that could occur with Ice for C++ when using collocation optimization and serialization

8 files changed, 35 insertions, 15 deletions

diff --git a/CHANGELOG-3.6.md b/CHANGELOG-3.6.md
index ed6df276f5e..44de903603a 100644
--- a/CHANGELOG-3.6.md
+++ b/CHANGELOG-3.6.md
@@ -36,7 +36,7 @@ These are the changes since Ice 3.6.1.
   IceGrid and IceStorm databases.
 
 - Fixed a bug that affects Java and C# generated code. The generated patcher for
-  reading class data members was bogus when the class had more than one class data 
+  reading class data members was bogus when the class had more than one class data
   member and derived from a class that contained class data members. The same
   issue was true for exceptions with class data members deriving from exceptions
   with class data members.
@@ -49,10 +49,13 @@ These are the changes since Ice 3.6.1.
 - Fixed El Capitan build issues caused by a new security feature that no longer
   exports DYLD_LIBRARY_PATH to child processes.
 
+- Fixed potential deadlock that could occur when using collocation
+  optimization and serialized server thread pools.
+
 ## C# Changes
 
-- Minor fixes to release the memory used by the transports to send and receive 
-  data sooner. This garbage collector can therefore collect this memory sooner 
+- Minor fixes to release the memory used by the transports to send and receive
+  data sooner. This garbage collector can therefore collect this memory sooner
   as well.
 
 # Changes in Ice 3.6.1
diff --git a/cpp/src/Ice/ThreadPool.cpp b/cpp/src/Ice/ThreadPool.cpp
index 7ff3f57db4b..5d03c88f4f4 100644
--- a/cpp/src/Ice/ThreadPool.cpp
+++ b/cpp/src/Ice/ThreadPool.cpp
@@ -777,7 +777,7 @@ IceInternal::ThreadPool::run(const EventHandlerThreadPtr& thread)
                     // If the handler called ioCompleted(), we re-enable the handler in
                     // case it was disabled and we decrease the number of thread in use.
                     //
-                    if(_serialize)
+                    if(_serialize && current._handler.get() != _workQueue.get())
                     {
                         _selector.enable(current._handler.get(), current.operation);
                         if(current._handler->_hasMoreData && current._handler->_registered & SocketOperationRead)
@@ -1004,7 +1004,7 @@ IceInternal::ThreadPool::ioCompleted(ThreadPoolCurrent& current)
 
         if(!_destroyed)
         {
-            if(_serialize)
+            if(_serialize && current._handler.get() != _workQueue.get())
             {
                 _selector.disable(current._handler.get(), current.operation);
 
@@ -1079,7 +1079,7 @@ IceInternal::ThreadPool::ioCompleted(ThreadPoolCurrent& current)
         }
     }
 
-    return _serialize;
+    return _serialize && current._handler.get() != _workQueue.get();
 }
 
 #if defined(ICE_USE_IOCP) || defined(ICE_OS_WINRT)
diff --git a/cpp/test/Ice/adapterDeactivation/AllTests.cpp b/cpp/test/Ice/adapterDeactivation/AllTests.cpp
index 8186540cb30..9587c0cc268 100644
--- a/cpp/test/Ice/adapterDeactivation/AllTests.cpp
+++ b/cpp/test/Ice/adapterDeactivation/AllTests.cpp
@@ -30,10 +30,10 @@ allTests(const CommunicatorPtr& communicator)
     cout << "ok" << endl;
 
     {
-        string host = communicator->getProperties()->getPropertyAsIntWithDefault("Ice.IPv6", 0) == 0 ? 
+        string host = communicator->getProperties()->getPropertyAsIntWithDefault("Ice.IPv6", 0) == 0 ?
             "127.0.0.1" : "\"0:0:0:0:0:0:0:1\"";
         cout << "creating/destroying/recreating object adapter... " << flush;
-        ObjectAdapterPtr adapter = 
+        ObjectAdapterPtr adapter =
             communicator->createObjectAdapterWithEndpoints("TransientTestAdapter", "default -h " + host);
         try
         {
@@ -55,6 +55,7 @@ allTests(const CommunicatorPtr& communicator)
 
     cout << "creating/activating/deactivating object adapter in one operation... " << flush;
     obj->transient();
+    obj->end_transient(obj->begin_transient());
     cout << "ok" << endl;
 
     {
diff --git a/cpp/test/Ice/adapterDeactivation/Collocated.cpp b/cpp/test/Ice/adapterDeactivation/Collocated.cpp
index ef093c73098..48763b3ba41 100644
--- a/cpp/test/Ice/adapterDeactivation/Collocated.cpp
+++ b/cpp/test/Ice/adapterDeactivation/Collocated.cpp
@@ -22,6 +22,12 @@ int
 run(int, char**, const Ice::CommunicatorPtr& communicator)
 {
     communicator->getProperties()->setProperty("TestAdapter.Endpoints", "default -p 12010");
+
+    //
+    // 2 threads are necessary to dispatch the collocated transient() call with AMI
+    //
+    communicator->getProperties()->setProperty("TestAdapter.ThreadPool.Size", "2");
+
     Ice::ObjectAdapterPtr adapter = communicator->createObjectAdapter("TestAdapter");
     ServantLocatorPtr locator = new ServantLocatorI;
     adapter->addServantLocator(locator, "");
diff --git a/csharp/test/Ice/adapterDeactivation/AllTests.cs b/csharp/test/Ice/adapterDeactivation/AllTests.cs
index 833faf409f5..cd325c348f8 100644
--- a/csharp/test/Ice/adapterDeactivation/AllTests.cs
+++ b/csharp/test/Ice/adapterDeactivation/AllTests.cs
@@ -73,6 +73,7 @@ public class AllTests : TestCommon.TestApp
         Write("creating/activating/deactivating object adapter in one operation... ");
         Flush();
         obj.transient();
+        obj.end_transient(obj.begin_transient());
         WriteLine("ok");
 
         {
diff --git a/csharp/test/Ice/adapterDeactivation/Collocated.cs b/csharp/test/Ice/adapterDeactivation/Collocated.cs
index f607c1a4954..d0ca641129d 100644
--- a/csharp/test/Ice/adapterDeactivation/Collocated.cs
+++ b/csharp/test/Ice/adapterDeactivation/Collocated.cs
@@ -25,17 +25,23 @@ public class Collocated
         public override int run(string[] args)
         {
             communicator().getProperties().setProperty("TestAdapter.Endpoints", "default -p 12010");
+
+            //
+            // 2 threads are necessary to dispatch the collocated transient() call with AMI
+            //
+            communicator().getProperties().setProperty("TestAdapter.ThreadPool.Size", "2");
+
             Ice.ObjectAdapter adapter = communicator().createObjectAdapter("TestAdapter");
             Ice.ServantLocator locator = new ServantLocatorI();
             adapter.addServantLocator(locator, "");
 
             AllTests.allTests(communicator());
-            
+
             adapter.waitForDeactivate();
             return 0;
         }
     }
-    
+
     public static int Main(string[] args)
     {
         App app = new App();
diff --git a/java/test/src/main/java/test/Ice/adapterDeactivation/AllTests.java b/java/test/src/main/java/test/Ice/adapterDeactivation/AllTests.java
index ea5cce04aeb..1b49cae4af9 100644
--- a/java/test/src/main/java/test/Ice/adapterDeactivation/AllTests.java
+++ b/java/test/src/main/java/test/Ice/adapterDeactivation/AllTests.java
@@ -44,7 +44,7 @@ public class AllTests
         {
             out.print("creating/destroying/recreating object adapter... ");
             out.flush();
-            Ice.ObjectAdapter adapter = 
+            Ice.ObjectAdapter adapter =
                 communicator.createObjectAdapterWithEndpoints("TransientTestAdapter", "default");
             try
             {
@@ -66,6 +66,7 @@ public class AllTests
         out.print("creating/activating/deactivating object adapter in one operation... ");
         out.flush();
         obj._transient();
+        obj.end_transient(obj.begin_transient());
         out.println("ok");
 
         {
diff --git a/java/test/src/main/java/test/Ice/adapterDeactivation/Collocated.java b/java/test/src/main/java/test/Ice/adapterDeactivation/Collocated.java
index 8609cd895a7..7ca338b74ab 100644
--- a/java/test/src/main/java/test/Ice/adapterDeactivation/Collocated.java
+++ b/java/test/src/main/java/test/Ice/adapterDeactivation/Collocated.java
@@ -28,10 +28,12 @@ public class Collocated extends test.Util.Application
     {
         Ice.InitializationData initData = createInitializationData();
         initData.properties = Ice.Util.createProperties(argsH);
-        if(initData.properties.getPropertyAsInt("Ice.ThreadInterruptSafe") > 0 || isAndroid())
-        {
-            initData.properties.setProperty("Ice.ThreadPool.Server.Size", "2");
-        }
+
+        //
+        // 2 threads are necessary to dispatch the collocated transient() call with AMI
+        //
+        initData.properties.setProperty("TestAdapter.ThreadPool.Size", "2");
+
         initData.properties.setProperty("Ice.Package.Test", "test.Ice.adapterDeactivation");
         initData.properties.setProperty("TestAdapter.Endpoints", "default -p 12010");
         return initData;