From 544866e18d95ead1153be5c474a32a5a3658815e Mon Sep 17 00:00:00 2001 From: irfan Date: Thu, 22 Feb 2001 23:47:15 +0000 Subject: ChangeLogTag: Thu Feb 22 12:28:15 2001 Irfan Pyarali --- ChangeLog | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ ChangeLogs/ChangeLog-02a | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ ChangeLogs/ChangeLog-03a | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ ace/Select_Reactor_T.i | 10 +++++---- ace/TP_Reactor.cpp | 17 ++++++++-------- ace/TP_Reactor.h | 3 --- ace/TP_Reactor.i | 12 ----------- ace/Token.cpp | 19 ----------------- ace/Token.h | 16 --------------- 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 + + * 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 + 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 + 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 + 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 * 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 + + * 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 + 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 + 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 + 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 * 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 + + * 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 + 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 + 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 + 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 * 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::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::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::alertable_handle_events (ACE_Tim template /* ACE_INLINE */ int ACE_Select_Reactor_T::deactivated (void) { + ACE_MT (ACE_GUARD_RETURN (ACE_SELECT_REACTOR_TOKEN, ace_mon, this->token_, -1)); return this->deactivated_; } template /* ACE_INLINE */ void ACE_Select_Reactor_T::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 , 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 @@ -15,18 +15,6 @@ ACE_TP_Reactor::mask_ops (ACE_Event_Handler *eh, return this->mask_ops (eh->get_handle (), mask, ops); } -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, 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 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 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_; }; -- cgit v1.2.1