bug 1718 - race condition in ConnectionI destructor for

    thread-per-connection

3 files changed, 71 insertions, 40 deletions

diff --git a/cpp/src/Ice/ConnectionFactory.cpp b/cpp/src/Ice/ConnectionFactory.cpp
index 3008dfaa485..aa5d713a623 100644
--- a/cpp/src/Ice/ConnectionFactory.cpp
+++ b/cpp/src/Ice/ConnectionFactory.cpp
@@ -294,6 +294,7 @@ IceInternal::OutgoingConnectionFactory::create(const vector<EndpointIPtr>& endpt
             }       
             connection = new ConnectionI(_instance, transceiver, endpoint, 0, threadPerConnection,
                                          _instance->threadPerConnectionStackSize());
+            connection->start();
             connection->validate();
 
             if(_instance->defaultsAndOverrides()->overrideCompress)
@@ -762,8 +763,9 @@ IceInternal::IncomingConnectionFactory::message(BasicStream&, const ThreadPoolPt
 
         try
         {
-            connection = new ConnectionI(_instance, transceiver, _endpoint, _adapter, _threadPerConnection,
-                                         _threadPerConnectionStackSize);
+            assert(!_threadPerConnection);
+            connection = new ConnectionI(_instance, transceiver, _endpoint, _adapter, false, 0);
+            connection->start();
         }
         catch(const LocalException&)
         {
@@ -872,6 +874,7 @@ IceInternal::IncomingConnectionFactory::IncomingConnectionFactory(const Instance
         {
             connection = new ConnectionI(_instance, _transceiver, _endpoint, _adapter, _threadPerConnection,
                                          _threadPerConnectionStackSize);
+            connection->start();
             connection->validate();
         }
         catch(const LocalException&)
@@ -1144,6 +1147,7 @@ IceInternal::IncomingConnectionFactory::run()
                 {
                     connection = new ConnectionI(_instance, transceiver, _endpoint, _adapter, _threadPerConnection,
                                                  _threadPerConnectionStackSize);
+                    connection->start();
                 }
                 catch(const LocalException&)
                 {
diff --git a/cpp/src/Ice/ConnectionI.cpp b/cpp/src/Ice/ConnectionI.cpp
index 22b4c28cf23..bb3052b86e1 100644
--- a/cpp/src/Ice/ConnectionI.cpp
+++ b/cpp/src/Ice/ConnectionI.cpp
@@ -1572,6 +1572,7 @@ Ice::ConnectionI::ConnectionI(const InstancePtr& instance,
                               size_t threadPerConnectionStackSize) :
     EventHandler(instance),
     _threadPerConnection(threadPerConnection),
+    _threadPerConnectionStackSize(threadPerConnectionStackSize),
     _transceiver(transceiver),
     _desc(transceiver->toString()),
     _type(transceiver->type()),
@@ -1633,17 +1634,17 @@ Ice::ConnectionI::ConnectionI(const InstancePtr& instance,
         _servantManager = adapterImpl->getServantManager();
     }
 
-    __setNoDelete(true);
-    try
+    if(!threadPerConnection)
     {
-        if(!threadPerConnection)
+        //
+        // Only set _threadPool if we really need it, i.e., if we are
+        // not in thread per connection mode. Thread pools have lazy
+        // initialization in Instance, and we don't want them to be
+        // created if they are not needed.
+        //
+        __setNoDelete(true);
+        try
         {
-            //
-            // Only set _threadPool if we really need it, i.e., if we are
-            // not in thread per connection mode. Thread pools have lazy
-            // initialization in Instance, and we don't want them to be
-            // created if they are not needed.
-            //
             if(adapterImpl)
             {
                 const_cast<ThreadPoolPtr&>(_threadPool) = adapterImpl->getThreadPool();
@@ -1654,41 +1655,22 @@ Ice::ConnectionI::ConnectionI(const InstancePtr& instance,
             }
             _threadPool->incFdsInUse();
         }
-        else
+        catch(const IceUtil::Exception& ex)
         {
-            //
-            // If we are in thread per connection mode, create the
-            // thread for this connection.
-            //
-            _thread = new ThreadPerConnection(this);
-            _thread->start(threadPerConnectionStackSize);
-        }
-    }
-    catch(const IceUtil::Exception& ex)
-    {
-        {
-            Error out(_logger);
-            if(threadPerConnection)
+            try
             {
-                out << "cannot create thread for connection:\n" << ex;
+                _transceiver->close();
             }
-            // Otherwise with thread pool the thread pool itself
-            // prints a warning if the threads cannot be created.
-        }
-        
-        try
-        {
-            _transceiver->close();
-        }
-        catch(const LocalException&)
-        {
-            // Here we ignore any exceptions in close().
+            catch(const LocalException&)
+            {
+                // Here we ignore any exceptions in close().
+            }
+
+            __setNoDelete(false);
+            ex.ice_throw();
         }
-        
         __setNoDelete(false);
-        ex.ice_throw();
     }
-    __setNoDelete(false);
 }
 
 Ice::ConnectionI::~ConnectionI()
@@ -1700,6 +1682,49 @@ Ice::ConnectionI::~ConnectionI()
 }
 
 void
+Ice::ConnectionI::start()
+{
+    //
+    // If we are in thread per connection mode, create the thread for this connection.
+    // We can't start the thread in the constructor because it can cause a race condition
+    // (see bug 1718).
+    //
+    if(_threadPerConnection)
+    {
+        try
+        {
+            _thread = new ThreadPerConnection(this);
+            _thread->start(_threadPerConnectionStackSize);
+        }
+        catch(const IceUtil::Exception& ex)
+        {
+            {
+                Error out(_logger);
+                out << "cannot create thread for connection:\n" << ex;
+            }
+
+            try
+            {
+                _transceiver->close();
+            }
+            catch(const LocalException&)
+            {
+                // Here we ignore any exceptions in close().
+            }
+
+            //
+            // Clean up.
+            //
+            _transceiver = 0;
+            _thread = 0;
+            _state = StateClosed;
+
+            ex.ice_throw();
+        }
+    }
+}
+
+void
 Ice::ConnectionI::setState(State state, const LocalException& ex)
 {
     //
diff --git a/cpp/src/Ice/ConnectionI.h b/cpp/src/Ice/ConnectionI.h
index f07841c5632..8e712cb3b42 100644
--- a/cpp/src/Ice/ConnectionI.h
+++ b/cpp/src/Ice/ConnectionI.h
@@ -105,6 +105,7 @@ private:
     ConnectionI(const IceInternal::InstancePtr&, const IceInternal::TransceiverPtr&, 
                 const IceInternal::EndpointIPtr&, const ObjectAdapterPtr&, bool, size_t);
     virtual ~ConnectionI();
+    void start();
     friend class IceInternal::IncomingConnectionFactory;
     friend class IceInternal::OutgoingConnectionFactory;
 
@@ -153,6 +154,7 @@ private:
     // Defined as mutable because "isFinished() const" sets this to 0.
     mutable IceUtil::ThreadPtr _thread;
     const bool _threadPerConnection;
+    const size_t _threadPerConnectionStackSize;
 
     IceInternal::TransceiverPtr _transceiver;
     const std::string _desc;