Fixed deadlock in connection binding code (ICE-5693)

8 files changed, 99 insertions, 40 deletions

diff --git a/cpp/src/Ice/CollocatedRequestHandler.cpp b/cpp/src/Ice/CollocatedRequestHandler.cpp
index 95c151ff6e2..0b08198726a 100644
--- a/cpp/src/Ice/CollocatedRequestHandler.cpp
+++ b/cpp/src/Ice/CollocatedRequestHandler.cpp
@@ -150,6 +150,18 @@ CollocatedRequestHandler::~CollocatedRequestHandler()
 {
 }
 
+RequestHandlerPtr
+CollocatedRequestHandler::connect()
+{
+    return this;
+}
+
+RequestHandlerPtr
+CollocatedRequestHandler::update(const RequestHandlerPtr& previousHandler, const RequestHandlerPtr& newHandler)
+{
+    return previousHandler.get() == this ? newHandler : this;
+}
+
 void
 CollocatedRequestHandler::prepareBatchRequest(BasicStream* os)
 {
diff --git a/cpp/src/Ice/CollocatedRequestHandler.h b/cpp/src/Ice/CollocatedRequestHandler.h
index 36462f80feb..a3ac1387045 100644
--- a/cpp/src/Ice/CollocatedRequestHandler.h
+++ b/cpp/src/Ice/CollocatedRequestHandler.h
@@ -41,9 +41,11 @@ class CollocatedRequestHandler : public RequestHandler, public ResponseHandler,
 public:
 
     CollocatedRequestHandler(const ReferencePtr&, const Ice::ObjectAdapterPtr&);
-
     virtual ~CollocatedRequestHandler();
 
+    virtual RequestHandlerPtr connect();
+    virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&);
+
     virtual void prepareBatchRequest(BasicStream*);
     virtual void finishBatchRequest(BasicStream*);
     virtual void abortBatchRequest();
diff --git a/cpp/src/Ice/ConnectRequestHandler.cpp b/cpp/src/Ice/ConnectRequestHandler.cpp
index 874858e1c7e..7a6b495a13f 100644
--- a/cpp/src/Ice/ConnectRequestHandler.cpp
+++ b/cpp/src/Ice/ConnectRequestHandler.cpp
@@ -91,19 +91,36 @@ ConnectRequestHandler::~ConnectRequestHandler()
 RequestHandlerPtr
 ConnectRequestHandler::connect()
 {
+    Ice::ObjectPrx proxy = _proxy;
+
     _reference->getConnection(this);
 
-    Lock sync(*this);
-    if(initialized())
+    try
     {
-        assert(_connection);
-        return new ConnectionRequestHandler(_reference, _connection, _compress);
+        Lock sync(*this);
+        if(!initialized())
+        {
+            _updateRequestHandler = true; // The proxy request handler will be updated when the connection is set.
+            return this;
+        }
     }
-    else
+    catch(const Ice::LocalException&)
     {
-        _updateRequestHandler = true; // The proxy request handler will be updated when the connection is set.
-        return this;
+        proxy->__setRequestHandler(this, 0);
+        throw;
     }
+
+    assert(_connection);
+
+    RequestHandlerPtr handler = new ConnectionRequestHandler(_reference, _connection, _compress);
+    proxy->__setRequestHandler(this, handler);
+    return handler;
+}
+
+RequestHandlerPtr
+ConnectRequestHandler::update(const RequestHandlerPtr& previousHandler, const RequestHandlerPtr& newHandler)
+{
+    return previousHandler.get() == this ? newHandler : this;
 }
 
 void
@@ -335,8 +352,6 @@ ConnectRequestHandler::setConnection(const Ice::ConnectionIPtr& connection, bool
     {
         Lock sync(*this);
         assert(!_exception.get() && !_connection);
-        assert(_updateRequestHandler || _requests.empty());
-
         _connection = connection;
         _compress = compress;
     }
@@ -362,8 +377,6 @@ ConnectRequestHandler::setException(const Ice::LocalException& ex)
 {
     Lock sync(*this);
     assert(!_initialized && !_exception.get());
-    assert(_updateRequestHandler || _requests.empty());
-
     _exception.reset(ex.ice_clone());
     _proxy = 0; // Break cyclic reference count.
 
diff --git a/cpp/src/Ice/ConnectRequestHandler.h b/cpp/src/Ice/ConnectRequestHandler.h
index ea86da211ff..095edda6123 100644
--- a/cpp/src/Ice/ConnectRequestHandler.h
+++ b/cpp/src/Ice/ConnectRequestHandler.h
@@ -35,7 +35,8 @@ public:
     ConnectRequestHandler(const ReferencePtr&, const Ice::ObjectPrx&);
     virtual ~ConnectRequestHandler();
 
-    RequestHandlerPtr connect();
+    virtual RequestHandlerPtr connect();
+    virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&);
 
     virtual void prepareBatchRequest(BasicStream*);
     virtual void finishBatchRequest(BasicStream*);
diff --git a/cpp/src/Ice/ConnectionRequestHandler.cpp b/cpp/src/Ice/ConnectionRequestHandler.cpp
index 4d9d746675d..43dda1b88ed 100644
--- a/cpp/src/Ice/ConnectionRequestHandler.cpp
+++ b/cpp/src/Ice/ConnectionRequestHandler.cpp
@@ -38,6 +38,40 @@ ConnectionRequestHandler::ConnectionRequestHandler(const ReferencePtr& reference
 {
 }
 
+RequestHandlerPtr
+ConnectionRequestHandler::connect()
+{
+    assert(false); // This request handler is only created after connection binding.
+    return 0;
+}
+
+RequestHandlerPtr
+ConnectionRequestHandler::update(const RequestHandlerPtr& previousHandler, const RequestHandlerPtr& newHandler)
+{
+    assert(previousHandler);
+    try
+    {
+        if(previousHandler.get() == this)
+        {
+            return newHandler;
+        }
+        else if(previousHandler->getConnection() == _connection)
+        {
+            //
+            // If both request handlers point to the same connection, we also
+            // update the request handler. See bug ICE-5489 for reasons why
+            // this can be useful.
+            //
+            return newHandler;
+        }
+    }
+    catch(const Ice::Exception&)
+    {
+        // Ignore.
+    }
+    return this;
+}
+
 void
 ConnectionRequestHandler::prepareBatchRequest(BasicStream* out)
 {
diff --git a/cpp/src/Ice/ConnectionRequestHandler.h b/cpp/src/Ice/ConnectionRequestHandler.h
index d8d4b1f2ae5..5ab5a4c9ea7 100644
--- a/cpp/src/Ice/ConnectionRequestHandler.h
+++ b/cpp/src/Ice/ConnectionRequestHandler.h
@@ -24,6 +24,9 @@ public:
     ConnectionRequestHandler(const ReferencePtr&, const Ice::ObjectPrx&);
     ConnectionRequestHandler(const ReferencePtr&, const Ice::ConnectionIPtr&, bool);
 
+    virtual RequestHandlerPtr connect();
+    virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&);
+
     virtual void prepareBatchRequest(BasicStream*);
     virtual void finishBatchRequest(BasicStream*);
     virtual void abortBatchRequest();
diff --git a/cpp/src/Ice/Proxy.cpp b/cpp/src/Ice/Proxy.cpp
index 96721bea8f7..2d7ef940fa7 100644
--- a/cpp/src/Ice/Proxy.cpp
+++ b/cpp/src/Ice/Proxy.cpp
@@ -1622,6 +1622,7 @@ operator<<(ostream& os, const ::IceProxy::Ice::Object& p)
 ::IceInternal::RequestHandlerPtr
 IceProxy::Ice::Object::__getRequestHandler()
 {
+    RequestHandlerPtr handler;
     if(_reference->getCacheConnection())
     {
         IceUtil::Mutex::Lock sync(_mutex);
@@ -1629,11 +1630,14 @@ IceProxy::Ice::Object::__getRequestHandler()
         {
             return _requestHandler;
         }
-        _requestHandler = createRequestHandler(); // async = true to avoid blocking with the proxy mutex locked.
-        return _requestHandler;
+        handler = createRequestHandler();
+        _requestHandler = handler;
     }
-
-    return createRequestHandler();
+    else
+    {
+        handler = createRequestHandler();
+    }
+    return handler->connect();
 }
 
 void
@@ -1643,28 +1647,16 @@ IceProxy::Ice::Object::__setRequestHandler(const ::IceInternal::RequestHandlerPt
     if(_reference->getCacheConnection())
     {
         IceUtil::Mutex::Lock sync(_mutex);
-        if(previous.get() == _requestHandler.get())
+        if(_requestHandler.get() != handler.get())
         {
-            _requestHandler = handler;
-        }
-        else if(previous && _requestHandler)
-        {
-            try
-            {
-                //
-                // If both request handlers point to the same connection, we also
-                // update the request handler. See bug ICE-5489 for reasons why
-                // this can be useful.
-                //
-                if(previous->getConnection() == _requestHandler->getConnection())
-                {
-                    _requestHandler = handler;
-                }
-            }
-            catch(const Exception&)
-            {
-                // Ignore
-            }
+            //
+            // Update the request handler only if "previous" is the same
+            // as the current request handler. This is called after
+            // connection binding by the connect request handler. We only
+            // replace the request handler if the current handler is the
+            // connect request handler.
+            //
+            _requestHandler = _requestHandler->update(previous, handler);
         }
     }
 }
@@ -1687,8 +1679,7 @@ IceProxy::Ice::Object::createRequestHandler()
         }
     }
 
-    ConnectRequestHandlerPtr handler = new ::IceInternal::ConnectRequestHandler(_reference, this);
-    return handler->connect();
+    return new ::IceInternal::ConnectRequestHandler(_reference, this);
 }
 
 void
diff --git a/cpp/src/Ice/RequestHandler.h b/cpp/src/Ice/RequestHandler.h
index 45cf917dd0a..73c3e818b56 100644
--- a/cpp/src/Ice/RequestHandler.h
+++ b/cpp/src/Ice/RequestHandler.h
@@ -57,6 +57,9 @@ public:
 
     virtual ~RequestHandler();
 
+    virtual RequestHandlerPtr connect() = 0;
+    virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&) = 0;
+
     virtual void prepareBatchRequest(BasicStream*) = 0;
     virtual void finishBatchRequest(BasicStream*) = 0;
     virtual void abortBatchRequest() = 0;