ChangeLogTag: Thu Feb 22 12:28:15 2001 Irfan Pyarali <irfan@cs.wustl.edu>

9 files changed, 173 insertions, 63 deletions

diff --git a/ChangeLog b/ChangeLog
index 4083582f4a3..d19df54db8c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,56 @@
+Thu Feb 22 12:28:15 2001  Irfan Pyarali  <irfan@cs.wustl.edu>
+
+	* ace/TP_Reactor.cpp (handle_events): Here is the description of
+	  how things were working and where the problem was: In the
+	  TP_Reactor there is one leader thread waiting in select() and
+	  the others waiting on the token.  During deactivation(), the
+	  <deactivation_> flag is set on the reactor, signal_all_threads()
+	  is called on the token, and an empty notification is send to the
+	  current leader thread.  signal_all_threads() sets the
+	  <signal_all_threads_> flag on the Token when there are waiters
+	  on the Token (does nothing if there are no waiters).  This
+	  scheme worked fine when there were leader and follower threads
+	  waiting.  When a new leader is chosen, it returns the magic
+	  number 2 from token.acquire_read() and hence exits gracefully
+	  from the handle_events().  However, the problem was that when
+	  some (or all) of the TP threads are busy running upcalls,
+	  signal_all_threads() was a no-op and the magic number 2 was not
+	  returned from token.acquire_read() and hence the exit from
+	  handle_events() was flagged as an error.
+
+	  The fix is as described below: 
+
+	  (1) ACE_Token::signal_all_threads() does not make sense.  The
+	  token is a mutex, not a condition variable.  Therefore, it does
+	  not make sense to have a signal_all_threads() methods.  Plus I
+	  believe that the method does not do as advertised.  Therefore, I
+	  removed this method from the token class.
+
+	  (2) There is no need to signal all the threads in the
+	  TP_Reactor.  Marking the Reactor as closed and signaling the
+	  leader thread is enough.  The leader thread will wake up to
+	  handle the empty event, the next leader will see that the
+	  Reactor is closed and exit gracefully.  This will continue until
+	  all the waiters are drained.
+
+	  (3) With the above change, there is no need to check for an
+	  magic returns from token.acquire_read().  When a thread gets the
+	  token, it checks the deactivation flag before proceeding.  If
+	  the Reactor is closed, it gracefully exits.  Otherwise, it
+	  continues as leader.
+
+	  (4) Since there is no need to call token.signal_all_threads(),
+	  ACE_TP_Reactor::wakeup_all_threads() can be removed since it is
+	  now the same as ACE_Select_Reactor_T::wakeup_all_threads().
+
+	  Note that this change should help with making
+	  Thread_Pool_Reactor_Test run without shutdown errors.
+	  
+	* ace/Select_Reactor_T.i (deactivate): The setting of the
+	  <deactivated_> flag and waking up of all the threads should be
+	  atomic.  I am being picky about this one but I think am I right.
+	  Also added the guard to the accessor.
+
 Thu Feb 22 08:51:42 2001  Darrell Brunsch <brunsch@uci.edu>
 
         * ace/OS.i:  
diff --git a/ChangeLogs/ChangeLog-02a b/ChangeLogs/ChangeLog-02a
index 4083582f4a3..d19df54db8c 100644
--- a/ChangeLogs/ChangeLog-02a
+++ b/ChangeLogs/ChangeLog-02a
@@ -1,3 +1,56 @@
+Thu Feb 22 12:28:15 2001  Irfan Pyarali  <irfan@cs.wustl.edu>
+
+	* ace/TP_Reactor.cpp (handle_events): Here is the description of
+	  how things were working and where the problem was: In the
+	  TP_Reactor there is one leader thread waiting in select() and
+	  the others waiting on the token.  During deactivation(), the
+	  <deactivation_> flag is set on the reactor, signal_all_threads()
+	  is called on the token, and an empty notification is send to the
+	  current leader thread.  signal_all_threads() sets the
+	  <signal_all_threads_> flag on the Token when there are waiters
+	  on the Token (does nothing if there are no waiters).  This
+	  scheme worked fine when there were leader and follower threads
+	  waiting.  When a new leader is chosen, it returns the magic
+	  number 2 from token.acquire_read() and hence exits gracefully
+	  from the handle_events().  However, the problem was that when
+	  some (or all) of the TP threads are busy running upcalls,
+	  signal_all_threads() was a no-op and the magic number 2 was not
+	  returned from token.acquire_read() and hence the exit from
+	  handle_events() was flagged as an error.
+
+	  The fix is as described below: 
+
+	  (1) ACE_Token::signal_all_threads() does not make sense.  The
+	  token is a mutex, not a condition variable.  Therefore, it does
+	  not make sense to have a signal_all_threads() methods.  Plus I
+	  believe that the method does not do as advertised.  Therefore, I
+	  removed this method from the token class.
+
+	  (2) There is no need to signal all the threads in the
+	  TP_Reactor.  Marking the Reactor as closed and signaling the
+	  leader thread is enough.  The leader thread will wake up to
+	  handle the empty event, the next leader will see that the
+	  Reactor is closed and exit gracefully.  This will continue until
+	  all the waiters are drained.
+
+	  (3) With the above change, there is no need to check for an
+	  magic returns from token.acquire_read().  When a thread gets the
+	  token, it checks the deactivation flag before proceeding.  If
+	  the Reactor is closed, it gracefully exits.  Otherwise, it
+	  continues as leader.
+
+	  (4) Since there is no need to call token.signal_all_threads(),
+	  ACE_TP_Reactor::wakeup_all_threads() can be removed since it is
+	  now the same as ACE_Select_Reactor_T::wakeup_all_threads().
+
+	  Note that this change should help with making
+	  Thread_Pool_Reactor_Test run without shutdown errors.
+	  
+	* ace/Select_Reactor_T.i (deactivate): The setting of the
+	  <deactivated_> flag and waking up of all the threads should be
+	  atomic.  I am being picky about this one but I think am I right.
+	  Also added the guard to the accessor.
+
 Thu Feb 22 08:51:42 2001  Darrell Brunsch <brunsch@uci.edu>
 
         * ace/OS.i:  
diff --git a/ChangeLogs/ChangeLog-03a b/ChangeLogs/ChangeLog-03a
index 4083582f4a3..d19df54db8c 100644
--- a/ChangeLogs/ChangeLog-03a
+++ b/ChangeLogs/ChangeLog-03a
@@ -1,3 +1,56 @@
+Thu Feb 22 12:28:15 2001  Irfan Pyarali  <irfan@cs.wustl.edu>
+
+	* ace/TP_Reactor.cpp (handle_events): Here is the description of
+	  how things were working and where the problem was: In the
+	  TP_Reactor there is one leader thread waiting in select() and
+	  the others waiting on the token.  During deactivation(), the
+	  <deactivation_> flag is set on the reactor, signal_all_threads()
+	  is called on the token, and an empty notification is send to the
+	  current leader thread.  signal_all_threads() sets the
+	  <signal_all_threads_> flag on the Token when there are waiters
+	  on the Token (does nothing if there are no waiters).  This
+	  scheme worked fine when there were leader and follower threads
+	  waiting.  When a new leader is chosen, it returns the magic
+	  number 2 from token.acquire_read() and hence exits gracefully
+	  from the handle_events().  However, the problem was that when
+	  some (or all) of the TP threads are busy running upcalls,
+	  signal_all_threads() was a no-op and the magic number 2 was not
+	  returned from token.acquire_read() and hence the exit from
+	  handle_events() was flagged as an error.
+
+	  The fix is as described below: 
+
+	  (1) ACE_Token::signal_all_threads() does not make sense.  The
+	  token is a mutex, not a condition variable.  Therefore, it does
+	  not make sense to have a signal_all_threads() methods.  Plus I
+	  believe that the method does not do as advertised.  Therefore, I
+	  removed this method from the token class.
+
+	  (2) There is no need to signal all the threads in the
+	  TP_Reactor.  Marking the Reactor as closed and signaling the
+	  leader thread is enough.  The leader thread will wake up to
+	  handle the empty event, the next leader will see that the
+	  Reactor is closed and exit gracefully.  This will continue until
+	  all the waiters are drained.
+
+	  (3) With the above change, there is no need to check for an
+	  magic returns from token.acquire_read().  When a thread gets the
+	  token, it checks the deactivation flag before proceeding.  If
+	  the Reactor is closed, it gracefully exits.  Otherwise, it
+	  continues as leader.
+
+	  (4) Since there is no need to call token.signal_all_threads(),
+	  ACE_TP_Reactor::wakeup_all_threads() can be removed since it is
+	  now the same as ACE_Select_Reactor_T::wakeup_all_threads().
+
+	  Note that this change should help with making
+	  Thread_Pool_Reactor_Test run without shutdown errors.
+	  
+	* ace/Select_Reactor_T.i (deactivate): The setting of the
+	  <deactivated_> flag and waking up of all the threads should be
+	  atomic.  I am being picky about this one but I think am I right.
+	  Also added the guard to the accessor.
+
 Thu Feb 22 08:51:42 2001  Darrell Brunsch <brunsch@uci.edu>
 
         * ace/OS.i:  
diff --git a/ace/Select_Reactor_T.i b/ace/Select_Reactor_T.i
index 6241c735589..8d0054a6219 100644
--- a/ace/Select_Reactor_T.i
+++ b/ace/Select_Reactor_T.i
@@ -58,8 +58,8 @@ ACE_Select_Reactor_T<ACE_SELECT_REACTOR_TOKEN>::register_handler (int signum,
 {
   ACE_TRACE ("ACE_Select_Reactor_T::register_handler");
   return this->signal_handler_->register_handler (signum,
-						  new_sh, new_disp,
-						  old_sh, old_disp);
+                                                  new_sh, new_disp,
+                                                  old_sh, old_disp);
 }
 
 #if defined (ACE_WIN32)
@@ -136,8 +136,8 @@ ACE_Select_Reactor_T<ACE_SELECT_REACTOR_TOKEN>::cancel_timer (long timer_id,
 {
   ACE_TRACE ("ACE_Select_Reactor_T::cancel_timer");
   return this->timer_queue_->cancel (timer_id,
-				     arg,
-				     dont_call_handle_close);
+                                     arg,
+                                     dont_call_handle_close);
 }
 
 // Performs operations on the "ready" bits.
@@ -224,12 +224,14 @@ ACE_Select_Reactor_T<ACE_SELECT_REACTOR_TOKEN>::alertable_handle_events (ACE_Tim
 template <class ACE_SELECT_REACTOR_TOKEN> /* ACE_INLINE */ int
 ACE_Select_Reactor_T<ACE_SELECT_REACTOR_TOKEN>::deactivated (void)
 {
+  ACE_MT (ACE_GUARD_RETURN (ACE_SELECT_REACTOR_TOKEN, ace_mon, this->token_, -1));
   return this->deactivated_;
 }
 
 template <class ACE_SELECT_REACTOR_TOKEN> /* ACE_INLINE */ void
 ACE_Select_Reactor_T<ACE_SELECT_REACTOR_TOKEN>::deactivate (int do_stop)
 {
+  ACE_MT (ACE_GUARD (ACE_SELECT_REACTOR_TOKEN, ace_mon, this->token_));
   this->deactivated_ = do_stop;
   this->wakeup_all_threads ();
 }
diff --git a/ace/TP_Reactor.cpp b/ace/TP_Reactor.cpp
index 6957e50a8e9..300a0d1c135 100644
--- a/ace/TP_Reactor.cpp
+++ b/ace/TP_Reactor.cpp
@@ -87,22 +87,22 @@ ACE_TP_Reactor::handle_events (ACE_Time_Value *max_wait_time)
   // Update the countdown to reflect time waiting for the token.
   countdown.update ();
 
-  switch (result)
+  // Check for timeouts and errors.
+  if (result == -1)
     {
-    case 2:
-      ACE_MT (this->token_.release ());
-      return 0;
-    case -1:
       if (errno == ETIME)
         return 0;
-      return -1;
+      else
+        return -1;
     }
 
-  // After acquiring the lock, check if we have been deactivated.
+  // After acquiring the lock, check if we have been deactivated.  If
+  // we are deactivated, simply return without handling further
+  // events.
   if (this->deactivated_)
     {
       ACE_MT (this->token_.release ());
-      return -1;
+      return 0;
     }
 
   // We got the lock, lets handle some events.  Note: this method will
@@ -143,7 +143,6 @@ ACE_TP_Reactor::handle_events (ACE_Time_Value *max_wait_time)
     }
 
   return result;
-
 }
 
 int
diff --git a/ace/TP_Reactor.h b/ace/TP_Reactor.h
index 4f5b8d76d7b..dab47eedda7 100644
--- a/ace/TP_Reactor.h
+++ b/ace/TP_Reactor.h
@@ -166,9 +166,6 @@ public:
   /// Called from handle events
   static void no_op_sleep_hook (void *);
 
-  /// Wake up all threads in waiting in the event loop
-  virtual void wakeup_all_threads (void);
-
   // = Any thread can perform a <handle_events>, override the owner()
   //   methods to avoid the overhead of setting the owner thread.
 
diff --git a/ace/TP_Reactor.i b/ace/TP_Reactor.i
index 64b5ad0c0ea..091124adbef 100644
--- a/ace/TP_Reactor.i
+++ b/ace/TP_Reactor.i
@@ -16,18 +16,6 @@ ACE_TP_Reactor::mask_ops (ACE_Event_Handler *eh,
 }
 
 ACE_INLINE void
-ACE_TP_Reactor::wakeup_all_threads (void)
-{
-  ACE_MT (this->token_.signal_all_threads (););
-
-  // Send a notification, but don't block if there's no one to receive
-  // it.
-  this->notify (0,
-                ACE_Event_Handler::NULL_MASK,
-                (ACE_Time_Value *) &ACE_Time_Value::zero);
-}
-
-ACE_INLINE void
 ACE_TP_Reactor::notify_handle (ACE_HANDLE,
                                ACE_Reactor_Mask,
                                ACE_Handle_Set &,
diff --git a/ace/Token.cpp b/ace/Token.cpp
index c51629f549b..748016d8dea 100644
--- a/ace/Token.cpp
+++ b/ace/Token.cpp
@@ -165,7 +165,6 @@ ACE_Token::ACE_Token (const ACE_TCHAR *name, void *any)
     in_use_ (0),
     waiters_ (0),
     nesting_level_ (0),
-    signal_all_threads_ (0),
     attributes_ (USYNC_THREAD)
 {
 //  ACE_TRACE ("ACE_Token::ACE_Token");
@@ -318,9 +317,6 @@ ACE_Token::shared_acquire (void (*sleep_hook_func)(void *),
   // If this is a normal wakeup, this thread should be runnable.
   ACE_ASSERT (my_entry.runable_);
 
-  if (this->signal_all_threads_ != 0)
-    return 2;
-
   return ret;
 }
 
@@ -473,9 +469,6 @@ ACE_Token::renew (int requeue_position,
   // Reinstate nesting level.
   this->nesting_level_ = save_nesting_level_;
 
-  if (this->signal_all_threads_ != 0)
-    return 2;
-
   return 0;
 }
 
@@ -524,7 +517,6 @@ ACE_Token::wakeup_next_waiter (void)
       this->readers_.head_ == 0)
     {
       // No more waiters...
-      this->signal_all_threads_ = 0;
       return;
     }
 
@@ -550,17 +542,6 @@ ACE_Token::wakeup_next_waiter (void)
   this->owner_ = queue->head_->thread_id_;
 }
 
-int
-ACE_Token::signal_all_threads (void)
-{
-  ACE_TRACE ("ACE_Token::release");
-  ACE_GUARD_RETURN (ACE_Thread_Mutex, ace_mon, this->lock_, -1);
-
-  if (this->waiters_ != 0)
-      this->signal_all_threads_ = 1;
-  return this->waiters_;
-}
-
 #endif /* ACE_HAS_THREADS */
 
 #if defined (ACE_HAS_EXPLICIT_TEMPLATE_INSTANTIATION)
diff --git a/ace/Token.h b/ace/Token.h
index 54835a1ef1f..c4fc805061b 100644
--- a/ace/Token.h
+++ b/ace/Token.h
@@ -169,19 +169,6 @@ public:
   /// Return the id of the current thread that owns the token.
   ACE_thread_t current_owner (void);
 
-  /**
-   * Force all threads waiting to acquire the token to return one by
-   * one.  The method sets the <signal_all_thread_> to non-zero if
-   * there're threads waiting, and returns the number of threads
-   * waiting.  If there's no thread waiting for the token, the call
-   * returns 0 and doesn't do anything.  The last thread releases the
-   * token also reset the <singal_all_thread_> flag to 0.  This means,
-   * any threads that try to acquire the token after the call is
-   * issued will also get "signaled" and the number of threads waiting
-   * the token is only a snapshot.
-   */
-  int signal_all_threads (void);
-
   /// Dump the state of an object.
   void dump (void) const;
 
@@ -282,9 +269,6 @@ private:
   /// Current nesting level.
   int nesting_level_;
 
-  /// Whether we are "signaling" all threads or not.
-  int signal_all_threads_;
-
   /// The attributes for the condition variables, optimizes lock time.
   ACE_Condition_Attributes attributes_;
 };