ChangeLog tag: Tue May 16 05:22:15 UTC 2006 Phil Mesnier <mesnier_p@ociweb.com>

4 files changed, 105 insertions, 43 deletions

diff --git a/TAO/ChangeLog b/TAO/ChangeLog
index 933f5e14e7f..e7095ea6f50 100644
--- a/TAO/ChangeLog
+++ b/TAO/ChangeLog
@@ -1,3 +1,47 @@
+Tue May 16 05:22:15 UTC 2006  Phil Mesnier  <mesnier_p@ociweb.com>
+
+        * tao/IIOP_Connector.cpp:
+
+          This is a potential fix for the Bug 2417 flaw. The problem is
+          that when using nonblocking connects, in conjunction with asynch
+          invocations, it is possible for a transport to be returned by
+          the connector even though the network connection has not
+          completed. For asynchronous invocations using the SYNCH_NONE
+          policy, this is appropriate, as request messages may be queued
+          for delivery if/when the connection completes.
+
+          Bug 2417 describes a scenario where such a nonblocking
+          connection attempt fails, but the actual failure happens after
+          the transport has already been returned to the caller. This
+          causes a problem because the underlying ACE connector framework
+          relies on "borrowing" the reference to the connection handler
+          during the time it is waiting for connections to complete or
+          fail. For blocked connects this is fine because either the
+          transport will be returned to the caller associated with a
+          completely established connection, or a failure will occur.
+
+          The issue for nonblocking connects is that when a transport is
+          returned associated with a pending connection, the existing
+          transport connector and protocol-specific connector end up
+          associating to referrers to the same connection handler, without
+          incrementing the reference count. The two are the transport
+          being returned and the ACE_NonBlock_Connection_Handler that is
+          actually registered with the reactor waiting for success or
+          failure on the pending connection.
+
+          When a connection completes OK, the NBCH surrenders its
+          reference to the connection handler, thus restoring parity, as
+          the transport and/or cache entry will still hold the remaining
+          references, and the count is OK. But when the connection fails,
+          the base connector ends up calling close() on the connection
+          handler which in turn decrements the reference count. This then
+          sets the stage for a later crash from an apparent double delete.
+
+        * tao/IIOP_Connection_Handler.cpp:
+        * tao/Transport_Connector.cpp:
+
+          Added some comments and cleaned up some whitespace.
+
 Mon May 15 22:25:23 UTC 2006  Phil Mesnier  <mesnier_p@ociweb.com>
 
         * tao/Bounded_Sequence_CDR_T.h:
diff --git a/TAO/tao/IIOP_Connection_Handler.cpp b/TAO/tao/IIOP_Connection_Handler.cpp
index d65d0ae017d..0f59383abe5 100644
--- a/TAO/tao/IIOP_Connection_Handler.cpp
+++ b/TAO/tao/IIOP_Connection_Handler.cpp
@@ -500,23 +500,23 @@ TAO_IIOP_Connection_Handler::set_dscp_codepoint (CORBA::Boolean set_network_prio
 void
 TAO_IIOP_Connection_Handler::abort (void)
 {
-      struct linger lval;
-      lval.l_onoff = 1;
-      lval.l_linger = 0;
-
-      if (this->peer ().set_option(SOL_SOCKET,
-                                   SO_LINGER,
-                                   (void*) &lval,
-                                   sizeof (lval)) == -1)
+  struct linger lval;
+  lval.l_onoff = 1;
+  lval.l_linger = 0;
+
+  if (this->peer ().set_option(SOL_SOCKET,
+                               SO_LINGER,
+                               (void*) &lval,
+                               sizeof (lval)) == -1)
+    {
+      if (TAO_debug_level)
         {
-          if (TAO_debug_level)
-            {
-              ACE_DEBUG ((LM_DEBUG,
-                          ACE_TEXT ("TAO (%P|%t) Unable to set ")
-                          ACE_TEXT ("SO_LINGER on %d\n"),
-                          this->peer ().get_handle ()));
-            }
+          ACE_DEBUG ((LM_DEBUG,
+                      ACE_TEXT ("TAO (%P|%t) Unable to set ")
+                      ACE_TEXT ("SO_LINGER on %d\n"),
+                      this->peer ().get_handle ()));
         }
+    }
 }
 
 
@@ -529,4 +529,3 @@ TAO_IIOP_Connection_Handler::abort (void)
 TAO_END_VERSIONED_NAMESPACE_DECL
 
 #endif /* TAO_HAS_IIOP && TAO_HAS_IIOP != 0 */
-
diff --git a/TAO/tao/IIOP_Connector.cpp b/TAO/tao/IIOP_Connector.cpp
index cd32ff04f90..ec2165a07e4 100644
--- a/TAO/tao/IIOP_Connector.cpp
+++ b/TAO/tao/IIOP_Connector.cpp
@@ -190,7 +190,7 @@ TAO_IIOP_Connector::make_connection (TAO::Profile_Transport_Resolver *r,
       // connect completed unsuccessfully
       svc_handler->remove_reference();
        // Give users a clue to the problem.
-      if (TAO_debug_level > 3)
+      if (TAO_debug_level > 1)
         {
           ACE_DEBUG ((LM_ERROR,
                       ACE_TEXT ("(%P|%t) IIOP_Connector::make_connection, ")
@@ -395,58 +395,63 @@ TAO_IIOP_Connector::complete_connection (int result,
     }
   else
     {
-          if (count == 1)
+      if (count == 1)
         {
-              transport = tlist[0];
+          transport = tlist[0];
           if (!this->wait_for_connection_completion (r,
                                                      transport,
                                                      timeout))
             {
               if (TAO_debug_level > 2)
-                    ACE_ERROR ((LM_ERROR,
-                                ACE_TEXT ("TAO (%P|%t) - IIOP_Connector::")
-                                ACE_TEXT ("complete_connection, wait for completion ")
-                                ACE_TEXT ("failed for 1 pending connect\n")));
+                ACE_ERROR ((LM_ERROR,
+                            ACE_TEXT ("TAO (%P|%t) - IIOP_Connector::")
+                            ACE_TEXT ("complete_connection, wait for completion ")
+                            ACE_TEXT ("failed for 1 pending connect\n")));
             }
         }
       else
         {
-              if (!this->wait_for_connection_completion (r,
-                                                         transport,
-                                                         tlist,
-                                                         count,
-                                                         mev,
-                                                         timeout))
-                {
-                  if (TAO_debug_level > 2)
-                    ACE_ERROR ((LM_ERROR,
-                                ACE_TEXT ("TAO (%P|%t) - IIOP_Connector::")
-                                ACE_TEXT ("complete_connection, wait for completion ")
-                                ACE_TEXT ("failed for %d pending connects\n"),
-                                count));
-                }
+          if (!this->wait_for_connection_completion (r,
+                                                     transport,
+                                                     tlist,
+                                                     count,
+                                                     mev,
+                                                     timeout))
+            {
+              if (TAO_debug_level > 2)
+                ACE_ERROR ((LM_ERROR,
+                            ACE_TEXT ("TAO (%P|%t) - IIOP_Connector::")
+                            ACE_TEXT ("complete_connection, wait for completion ")
+                            ACE_TEXT ("failed for %d pending connects\n"),
+                            count));
             }
         }
+    }
   // At this point, the connection has be successfully created
   // connected or not connected, but we have a connection.
   TAO_IIOP_Connection_Handler *svc_handler = 0;
   TAO_IIOP_Endpoint *iiop_endpoint = 0;
 
   if (transport != 0)
-        {
+    {
       for (unsigned i = 0; i < count; i++)
         {
           if (transport == tlist[i])
             {
               svc_handler = sh_list[i];
+              if (transport->connection_handler()->keep_waiting())
+                {
+                  svc_handler->add_reference();
+                }
               iiop_endpoint = ep_list[i];
               break;
-        }
+            }
         }
     }
 
 
-  // Done with the transport list
+  // Done with the transport list. It was a temporary that did not
+  // affect the reference count.
   delete [] tlist;
 
   // In case of errors transport is zero
@@ -631,7 +636,7 @@ TAO_IIOP_Connector::cancel_svc_handler (
   if (handler)
     {
       handler->abort();
-    return this->base_connector_.cancel (handler);
+      return this->base_connector_.cancel (handler);
     }
 
   return -1;
diff --git a/TAO/tao/Transport_Connector.cpp b/TAO/tao/Transport_Connector.cpp
index 61acac6efe5..82aebdf3c21 100644
--- a/TAO/tao/Transport_Connector.cpp
+++ b/TAO/tao/Transport_Connector.cpp
@@ -393,6 +393,7 @@ TAO_Connector::connect (TAO::Profile_Transport_Resolver *r,
                   "TAO_UNSPECIFIED_ROLE" ));
     }
 
+
   // If connected return.
   if (base_transport->is_connected ())
     return base_transport;
@@ -400,6 +401,14 @@ TAO_Connector::connect (TAO::Profile_Transport_Resolver *r,
   // It it possible to get a transport from the cache that is not
   // connected? If not, then the following code is bogus. We cannot
   // wait for a connection to complete on a transport in the cache.
+  //
+  // (mesnier_p@ociweb.com) It is indeed possible to reach this point.
+  // The AMI_Buffering test does. When using non-blocking connects and
+  // the first request(s) are asynch and may be queued, the connection
+  // establishment may not be completed by the time the invocation is
+  // done with it. In that case it is up to a subsequent invocation to
+  // handle the connection completion.
+
   if (!this->wait_for_connection_completion (r,
                                              base_transport,
                                              timeout))
@@ -445,7 +454,8 @@ TAO_Connector::wait_for_connection_completion (
 {
   if (TAO_debug_level > 2)
       ACE_DEBUG ((LM_DEBUG,
-                  "TAO (%P|%t) - Transport_Connector::wait_for_connection_completion, "
+                  "TAO (%P|%t) - Transport_Connector::"
+                  "wait_for_connection_completion, "
                   "going to wait for connection completion on transport"
                   "[%d]\n",
                   transport->id ()));
@@ -514,7 +524,10 @@ TAO_Connector::wait_for_connection_completion (
                         "transport [%d], wait for completion failed\n",
                         transport->id()));
 
-          // Set transport to zero, it is not usable
+
+          // Set transport to zero, it is not usable, and the reference
+          // count we added above was decremented by the base connector
+          // handling the connection failure.
           transport = 0;
 
           return false;
@@ -680,6 +693,7 @@ TAO_Connector::check_connection_closure (
       // handler.
       closed = connection_handler->is_closed ();
 
+
       // If closed, there is nothing to do here.  If not closed,
       // it was either opened or is still pending.
       if (!closed)