ChangeLogTag: Mon Dec 23 22:33:56 2002 Balachandran Natarajan <bala@isis-server.isis.vanderbilt.edu>

7 files changed, 53 insertions, 202 deletions

diff --git a/TAO/ChangeLog b/TAO/ChangeLog
index 56ecb606736..b4ba3baf4d8 100644
--- a/TAO/ChangeLog
+++ b/TAO/ChangeLog
@@ -1,3 +1,34 @@
+Mon Dec 23 22:33:56 2002  Balachandran Natarajan  <bala@isis-server.isis.vanderbilt.edu>
+
+	A slight redesign of the changes applied to fix BUG 1276. The
+        redesign was motivated due to a  drop in our performance. 
+	
+	* tao/Reply_Dispatcher.cpp:
+	* tao/Reply_Dispatcher.h:
+	* tao/Reply_Dispatcher.i: Did a slight redesign of how timeouts are
+	  handled.  This was necessitated since the previous design made
+	  things slow. Though none of the synchronization primitives were
+	  used for normal cases ie. invocations without timeouts, we still
+	  created them for every invocation. Creation of synchronizaton
+	  primitives actually made things slow, and they have now been
+	  removed. 
+	
+	* tao/Muxed_TMS.cpp: Removed calls to start_dispatch () and
+	  end_dispatch () on the Reply_Dispatcher. 
+	
+	* tao/Invocation.cpp: We now do a synchronization on the internal
+	  lock used by Muxed_TMS strategy so that no two threads are
+	  active on the Reply_Handler at the same time. This, though not
+	  very elegant, provides the same effect as the changes in "Sun
+	  Dec 22 11:26:30 2002  Balachandran Natarajan
+	  <bala@isis-server.isis.vanderbilt.edu>". The gist of the changes
+	  mentioned above have still been retained. 
+
+	* tao/Exclusive_TMS.cpp: Removed calls to start_dispatch and
+	  end_dispatch () on the Reply_Dispatcher. 
+	
+	  These changes should get our performance back on track.
+
 Mon Dec 23 18:21:51 2002  Pradeep Gore  <pradeep@oomworks.com>
 
  	* orbsvcs/tests/Notify/Blocking/Notify_Structured_Push_Consumer.cpp:
diff --git a/TAO/tao/Exclusive_TMS.cpp b/TAO/tao/Exclusive_TMS.cpp
index 17fa3990127..8b643c07db7 100644
--- a/TAO/tao/Exclusive_TMS.cpp
+++ b/TAO/tao/Exclusive_TMS.cpp
@@ -95,17 +95,9 @@ TAO_Exclusive_TMS::dispatch_reply (TAO_Pluggable_Reply_Params &params)
   this->request_id_ = 0; // @@ What is a good value???
   this->rd_ = 0;
 
-  // Starting dispatch
-  (void) rd->start_dispatch ();
-
   // Dispatch the reply.
   // Returns 1 on success, -1 on failure.
-  int retval =
-    rd->dispatch_reply (params);
-
-  (void) rd->end_dispatch ();
-
-  return retval;
+  return rd->dispatch_reply (params);
 }
 
 int
diff --git a/TAO/tao/Invocation.cpp b/TAO/tao/Invocation.cpp
index 7d12897caa3..75cbe6ec6c0 100644
--- a/TAO/tao/Invocation.cpp
+++ b/TAO/tao/Invocation.cpp
@@ -614,17 +614,6 @@ TAO_GIOP_Synch_Invocation::invoke_i (CORBA::Boolean is_locate_request
                         TAO_INVOKE_EXCEPTION);
     }
 
-  // Do we have timeout se for the invocation?
-  if (this->max_wait_time_ != 0)
-    {
-      if (TAO_debug_level > 4)
-        ACE_DEBUG ((LM_DEBUG,
-                    "TAO (%P|%t) - Synch_Invocation::invoke_i, "
-                    "setting timeout in the reply dispatcher \n"));
-
-      this->rd_.has_timeout (TAO_Reply_Dispatcher::TIMEOUT);
-    }
-
   // Just send the request, without trying to wait for the reply.
   int retval = TAO_GIOP_Invocation::invoke (TAO_Transport::TAO_TWOWAY_REQUEST
                                             ACE_ENV_ARG_PARAMETER);
@@ -850,10 +839,17 @@ TAO_GIOP_Synch_Invocation::validate_error (TAO_Bind_Dispatcher_Guard &guard
                 "TAO (%P|%t) - Synch_Invocation::invoke_i, "
                 "unbinding dispatcher after an error \n"));
 
-  int retval =
-    guard.unbind_dispatcher ();
-
-  if (retval == 0)
+  // If the unbind succeeds then thrown an exception to the
+  // application, else just collect the reply and dispatch that to the
+  // application.
+  // NOTE: A fragile synchronization is provided when using the Muxed
+  // Transport strategy. We could infact be a follower thread getting
+  // timedout in the LF whereas the dispatching thread could be
+  // on the reply_dispatcher that we created. This would lead bad
+  // crashes. To get around that, the call to unbind_dispatcher ()
+  // will wait on the lock on the Muxed_Transport_Strategy if
+  // dispatching has started. This is fragile.
+  if (guard.unbind_dispatcher () == 0)
     {
       // Just a timeout, don't close the connection or
       // anything...
@@ -865,17 +861,6 @@ TAO_GIOP_Synch_Invocation::validate_error (TAO_Bind_Dispatcher_Guard &guard
             TAO_INVOKE_EXCEPTION);
     }
 
-  if (TAO_debug_level > 0)
-    {
-      ACE_DEBUG ((LM_DEBUG,
-                  "TAO (%P|%t) - Synch_Invocation::validate_error "
-                  "waiting for dispatching to end \n"));
-    }
-
-  // Peek into the dispatcher to see whether we need to be waiting and
-  // if so wait
-  (void) this->rd_.wait_for_dispatch_completion ();
-
   return 0;
 }
 
diff --git a/TAO/tao/Muxed_TMS.cpp b/TAO/tao/Muxed_TMS.cpp
index a046c8cc948..22a4d97ad03 100644
--- a/TAO/tao/Muxed_TMS.cpp
+++ b/TAO/tao/Muxed_TMS.cpp
@@ -97,9 +97,6 @@ TAO_Muxed_TMS::dispatch_reply (TAO_Pluggable_Reply_Params &params)
   int result = 0;
   TAO_Reply_Dispatcher *rd = 0;
 
-  // Does the reply_dispatcher have a timeout?
-  CORBA::Boolean has_timeout = 0;
-
   // Grab the reply dispatcher for this id.
   {
     ACE_GUARD_RETURN (TAO_SYNCH_MUTEX, ace_mon, this->lock_, -1);
@@ -148,30 +145,17 @@ TAO_Muxed_TMS::dispatch_reply (TAO_Pluggable_Reply_Params &params)
         return 0;
       }
 
-    has_timeout = rd->has_timeout ();
-
-    if (has_timeout == TAO_Reply_Dispatcher::TIMEOUT)
-      {
-        // Only when the RD has a timeout it makes sense to let other
-        // thread know that we have started dispatching.
-        // Just let the Reply_Dispatcher know that dispatching has
-        // started.
-        (void) rd->start_dispatch ();
-      }
+    // Do not move it outside the scope of the lock. A follower thread
+    // could have timedout unwinding teh stack and the reply
+    // dispatcher and that would mean the present thread could be left
+    // with a dangling pointer and may crash. To safeguard againt such
+    // cases we dispatch with the lock held.
+    // Dispatch the reply.
+    // They return 1 on success, and -1 on failure.
+    result =  rd->dispatch_reply (params);
   }
 
-  // Dispatch the reply.
-  // They return 1 on success, and -1 on failure.
-  int retval =  rd->dispatch_reply (params);
-
-  if (has_timeout == TAO_Reply_Dispatcher::TIMEOUT)
-    {
-      // Only when the RD has a timeout it makes sense to let other
-      // thread know that we have finished dispatching.
-      // Just let the Reply_Dispatcher know that dispatching is done.
-      (void) rd->end_dispatch ();
-    }
-  return retval;
+  return result;
 }
 
 int
diff --git a/TAO/tao/Reply_Dispatcher.cpp b/TAO/tao/Reply_Dispatcher.cpp
index 86e0723344f..adde1dad792 100644
--- a/TAO/tao/Reply_Dispatcher.cpp
+++ b/TAO/tao/Reply_Dispatcher.cpp
@@ -12,12 +12,7 @@ ACE_RCSID(tao, Reply_Dispatcher, "$Id$")
 // Constructor.
 TAO_Reply_Dispatcher::TAO_Reply_Dispatcher (void)
   // Just an invalid reply status.
-  : reply_status_ (100),
-    mutex_ (),
-    condition_ (this->mutex_),
-    timeout_ (TAO_Reply_Dispatcher::NO_TIMEOUT),
-    dispatching_ (0),
-    threads_waiting_ (0)
+  : reply_status_ (100)
 {
 }
 
@@ -25,66 +20,3 @@ TAO_Reply_Dispatcher::TAO_Reply_Dispatcher (void)
 TAO_Reply_Dispatcher::~TAO_Reply_Dispatcher (void)
 {
 }
-
-void
-TAO_Reply_Dispatcher::start_dispatch (void)
-{
-  if (this->timeout_ != TAO_Reply_Dispatcher::TIMEOUT)
-    return;
-
-  {
-    ACE_MT (ACE_GUARD (ACE_SYNCH_MUTEX,
-                       guard,
-                       this->mutex_));
-
-    this->dispatching_ = 1;
-  }
-}
-
-
-void
-TAO_Reply_Dispatcher::end_dispatch (void)
-{
-  if (this->timeout_ != TAO_Reply_Dispatcher::TIMEOUT)
-    return;
-  {
-    ACE_MT (ACE_GUARD (ACE_SYNCH_MUTEX,
-                       ace_mon,
-                       this->mutex_));
-
-    this->dispatching_ = 0;
-
-    if (this->threads_waiting_)
-      this->condition_.signal ();
-  }
-}
-
-int
-TAO_Reply_Dispatcher::wait_for_dispatch_completion (void)
-{
-  if (this->timeout_ != TAO_Reply_Dispatcher::TIMEOUT)
-    return -1;
-
-  if (this->dispatching_)
-    {
-      ACE_MT (ACE_GUARD_RETURN (ACE_SYNCH_MUTEX,
-                                ace_mon,
-                                this->mutex_,
-                                -1));
-
-      // The dispatching could have ended by now, in which case just
-      // return.
-      if (this->dispatching_ == 0)
-        return 0;
-
-      // Mark the number of waiting threads
-      ++this->threads_waiting_;
-
-      this->condition_.wait ();
-
-      --this->threads_waiting_;
-      return 0;
-    }
-
-  return -1;
-}
diff --git a/TAO/tao/Reply_Dispatcher.h b/TAO/tao/Reply_Dispatcher.h
index 3e4dac7a424..bfbac94fa44 100644
--- a/TAO/tao/Reply_Dispatcher.h
+++ b/TAO/tao/Reply_Dispatcher.h
@@ -83,70 +83,9 @@ public:
   /// Get the reply status.
   CORBA::ULong reply_status (void) const;
 
-  /// Following methods are useful only when the invocation has a
-  /// timeout.
-  /**
-   * MT invocations with timeouts are a different beast in
-   * itself. They need some special attention. Things get nasty when
-   * the leader thread collects the reply and at the same time the
-   * follower timesout waiting for the reply. There is a semantic
-   * problem here. Does the follower throw an exception back as a
-   * CORBA::TIMEOUT or just make one last check to see whether anyone
-   * (leader) has collected the reply. Some of these extra methods
-   * here help the invocation threads to make a decision one way or
-   * another. The code is designed to be on the positive side ie. the
-   * follower will collect the reply if a leader has collected its
-   * reply already, instead of throwing an exception.  Its a decision
-   * that can be argued for hours.
-   *
-   * The following methods should be no-ops if timeouts arent set.
-   */
-  enum
-    {
-      TIMEOUT = 0,
-      NO_TIMEOUT
-    };
-
-  /// Set whether the dispatcher should be prepared for a timeout. The
-  /// set operation is not synchronized.
-  void has_timeout (CORBA::Boolean t);
-
-  /// Accessor for <timeout_> and un_synchronized!
-  CORBA::Boolean has_timeout (void) const;
-
-  /// Methods used to change the state to indicate that whether
-  /// dispatching is started or not!
-  void start_dispatch (void);
-  void end_dispatch (void);
-
-  /// Wait on the condition variable for the dispatch to end. If no
-  /// threads are dispatching will just return immediately.
-  int wait_for_dispatch_completion (void);
-
 protected:
   /// Reply or LocateReply status.
   CORBA::ULong reply_status_;
-
-  /// Mutex and Synch condition for timeouts..
-  TAO_SYNCH_MUTEX mutex_;
-  TAO_Condition<TAO_SYNCH_MUTEX> condition_;
-
-  /// Flag to indicate whether the invocation for which this
-  /// dispatcher is used, has a timeout or not.
-  CORBA::Boolean timeout_;
-
-  /// Variable to indicate whether dispatching is happening in this
-  /// reply dispatcher.
-  /**
-   * Point to note is that, the value of the flag is not of much
-   * importance for the normal case, but only for the  case when
-   * timeouts are set.
-   */
-  CORBA::Boolean dispatching_;
-
-  /// Any threads waiting for end of dispatch?
-  CORBA::Boolean threads_waiting_;
-
 };
 
 #if defined (__ACE_INLINE__)
diff --git a/TAO/tao/Reply_Dispatcher.i b/TAO/tao/Reply_Dispatcher.i
index 9af1da59538..549263801ff 100644
--- a/TAO/tao/Reply_Dispatcher.i
+++ b/TAO/tao/Reply_Dispatcher.i
@@ -5,15 +5,3 @@ TAO_Reply_Dispatcher::reply_status (void) const
 {
   return this->reply_status_;
 }
-
-ACE_INLINE void
-TAO_Reply_Dispatcher::has_timeout (CORBA::Boolean t)
-{
-  this->timeout_ = t;
-}
-
-ACE_INLINE CORBA::Boolean
-TAO_Reply_Dispatcher::has_timeout (void) const
-{
-  return this->timeout_;
-}