ChangeLogTag:Wed Jun 19 14:25:52 2002 Balachandran Natarajan <bala@cs.wustl.edu>

6 files changed, 141 insertions, 44 deletions

diff --git a/ChangeLog b/ChangeLog
index 526612bda76..81b7723797a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,24 @@
+Wed Jun 19 14:25:52 2002  Balachandran Natarajan  <bala@cs.wustl.edu>
+
+	* ace/TP_Reactor.cpp (handle_socket_events): Couple of small
+	  fixes. The fixes are for 
+
+	  1. Bug http://deuce.doc.wustl.edu/bugzilla/show_bug.cgi?id=1231
+	  and it is self-explanatory
+
+	  2. The find () operation after dispatch was not holding the lock
+	  and chances of race are high. This is documented as #2 in 
+  	  bug http://deuce.doc.wustl.edu/bugzilla/show_bug.cgi?id=1233
+
+	  Added some comments in the code explaining what is going
+	  on. Thanks to Carlos for identifying the problem.
+
+	* ace/Event_Handler.h: Added a enum to indicate the two states
+	  returnable by the call resume_handler (). 
+	  
+	* ace/Event_Handler.cpp (resume_handler): Instead of returning 0,
+	  returned ACE_Event_Handler::ACE_REACTOR_RESUMES_HANDLER. 
+
 Wed Jun 19 13:42:16 2002  Balachandran Natarajan  <bala@cs.wustl.edu>
 
 	* tests/tests.dsw (Project): Added TP_Reactor_Test.dsp to the
diff --git a/ChangeLogs/ChangeLog-02a b/ChangeLogs/ChangeLog-02a
index 526612bda76..81b7723797a 100644
--- a/ChangeLogs/ChangeLog-02a
+++ b/ChangeLogs/ChangeLog-02a
@@ -1,3 +1,24 @@
+Wed Jun 19 14:25:52 2002  Balachandran Natarajan  <bala@cs.wustl.edu>
+
+	* ace/TP_Reactor.cpp (handle_socket_events): Couple of small
+	  fixes. The fixes are for 
+
+	  1. Bug http://deuce.doc.wustl.edu/bugzilla/show_bug.cgi?id=1231
+	  and it is self-explanatory
+
+	  2. The find () operation after dispatch was not holding the lock
+	  and chances of race are high. This is documented as #2 in 
+  	  bug http://deuce.doc.wustl.edu/bugzilla/show_bug.cgi?id=1233
+
+	  Added some comments in the code explaining what is going
+	  on. Thanks to Carlos for identifying the problem.
+
+	* ace/Event_Handler.h: Added a enum to indicate the two states
+	  returnable by the call resume_handler (). 
+	  
+	* ace/Event_Handler.cpp (resume_handler): Instead of returning 0,
+	  returned ACE_Event_Handler::ACE_REACTOR_RESUMES_HANDLER. 
+
 Wed Jun 19 13:42:16 2002  Balachandran Natarajan  <bala@cs.wustl.edu>
 
 	* tests/tests.dsw (Project): Added TP_Reactor_Test.dsp to the
diff --git a/ChangeLogs/ChangeLog-03a b/ChangeLogs/ChangeLog-03a
index 526612bda76..81b7723797a 100644
--- a/ChangeLogs/ChangeLog-03a
+++ b/ChangeLogs/ChangeLog-03a
@@ -1,3 +1,24 @@
+Wed Jun 19 14:25:52 2002  Balachandran Natarajan  <bala@cs.wustl.edu>
+
+	* ace/TP_Reactor.cpp (handle_socket_events): Couple of small
+	  fixes. The fixes are for 
+
+	  1. Bug http://deuce.doc.wustl.edu/bugzilla/show_bug.cgi?id=1231
+	  and it is self-explanatory
+
+	  2. The find () operation after dispatch was not holding the lock
+	  and chances of race are high. This is documented as #2 in 
+  	  bug http://deuce.doc.wustl.edu/bugzilla/show_bug.cgi?id=1233
+
+	  Added some comments in the code explaining what is going
+	  on. Thanks to Carlos for identifying the problem.
+
+	* ace/Event_Handler.h: Added a enum to indicate the two states
+	  returnable by the call resume_handler (). 
+	  
+	* ace/Event_Handler.cpp (resume_handler): Instead of returning 0,
+	  returned ACE_Event_Handler::ACE_REACTOR_RESUMES_HANDLER. 
+
 Wed Jun 19 13:42:16 2002  Balachandran Natarajan  <bala@cs.wustl.edu>
 
 	* tests/tests.dsw (Project): Added TP_Reactor_Test.dsp to the
diff --git a/ace/Event_Handler.cpp b/ace/Event_Handler.cpp
index 2ee5276d733..ad71ab47f78 100644
--- a/ace/Event_Handler.cpp
+++ b/ace/Event_Handler.cpp
@@ -140,7 +140,7 @@ ACE_Event_Handler::resume_handler (void)
 
   // Return a default value and allow the reactor to take care of
   // resuming the handler
-  return 0;
+  return ACE_Event_Handler::ACE_REACTOR_RESUMES_HANDLER;
 }
 
 
@@ -201,9 +201,9 @@ ACE_Event_Handler::read_adapter (void *args)
 
 int
 ACE_Event_Handler::register_stdin_handler (ACE_Event_Handler *eh,
-					   ACE_Reactor *reactor,
-					   ACE_Thread_Manager *thr_mgr,
-					   int flags)
+                                           ACE_Reactor *reactor,
+                                           ACE_Thread_Manager *thr_mgr,
+                                           int flags)
 {
 #if defined (ACE_WIN32) || defined (ACE_PSOS)
   ACE_UNUSED_ARG (reactor);
@@ -222,7 +222,7 @@ ACE_Event_Handler::register_stdin_handler (ACE_Event_Handler *eh,
 
 int
 ACE_Event_Handler::remove_stdin_handler (ACE_Reactor *reactor,
-					 ACE_Thread_Manager *thr_mgr)
+                                         ACE_Thread_Manager *thr_mgr)
 {
 #if defined (ACE_WIN32)
   ACE_UNUSED_ARG (reactor);
@@ -246,7 +246,7 @@ ACE_Notification_Buffer::ACE_Notification_Buffer (void)
 }
 
 ACE_Notification_Buffer::ACE_Notification_Buffer (ACE_Event_Handler *eh,
-						  ACE_Reactor_Mask mask)
+                                                  ACE_Reactor_Mask mask)
   : eh_ (eh),
     mask_ (mask)
 {
diff --git a/ace/Event_Handler.h b/ace/Event_Handler.h
index 87cf8aa37c7..80bcfe69bda 100644
--- a/ace/Event_Handler.h
+++ b/ace/Event_Handler.h
@@ -130,12 +130,21 @@ public:
   /// when a Win32 object becomes signaled).
   virtual int handle_signal (int signum, siginfo_t * = 0, ucontext_t * = 0);
 
-  /// Called to figure out whether the handler needs to resumed by the
-  /// reactor or the application can take care of it. The default
-  /// value of 0 would be returned which would allow the reactor to
-  /// take care of resumption of the handler. The application can
-  /// return a value more than zero and decide to resume the handler
-  /// themseleves.
+  enum
+    {
+      ACE_REACTOR_RESUMES_HANDLER = 0,
+      ACE_APPLICATION_RESUMES_HANDLER
+    };
+  /* Called to figure out whether the handler needs to resumed by the
+   * reactor or the application can take care of it. The default
+   * value of 0 would be returned which would allow the reactor to
+   * take care of resumption of the handler. The application can
+   * return a value more than zero and decide to resume the handler
+   * themseleves.
+   */
+  // @@ NOTE: This method is only useful for the ACE_TP_Reactor. Sad
+  // that we have to have this method in a class that is supposed to
+  // be used across different componets in ACE.
   virtual int resume_handler (void);
 
   virtual int handle_qos (ACE_HANDLE = ACE_INVALID_HANDLE);
diff --git a/ace/TP_Reactor.cpp b/ace/TP_Reactor.cpp
index 5e280861075..f631bc2057a 100644
--- a/ace/TP_Reactor.cpp
+++ b/ace/TP_Reactor.cpp
@@ -524,57 +524,82 @@ ACE_TP_Reactor::handle_socket_events (int &event_count,
                                       ACE_TP_Token_Guard &guard)
 {
 
-  // We got the lock, lets handle some events.  Note: this method will
-  // *not* dispatch any I/O handlers.  It will dispatch signals,
-  // timeouts, and notifications.
+  // We got the lock, lets handle some I/O events.
   ACE_EH_Dispatch_Info dispatch_info;
 
   this->get_socket_event_info (dispatch_info);
 
   // If there is any event handler that is ready to be dispatched, the
   // dispatch information is recorded in dispatch_info.
-  if (dispatch_info.dispatch ())
+  if (!dispatch_info.dispatch ())
     {
-      // Suspend the handler so that other threads don't start
-      // dispatching it.
-      // NOTE: This check was performed in older versions of the
-      // TP_Reactor. Looks like it is a waste..
-      if (dispatch_info.event_handler_ != this->notify_handler_)
-        this->suspend_i (dispatch_info.handle_);
+      return 0;
     }
 
+  // Suspend the handler so that other threads don't start
+  // dispatching it.
+  // NOTE: This check was performed in older versions of the
+  // TP_Reactor. Looks like it is a waste..
+  if (dispatch_info.event_handler_ != this->notify_handler_)
+    this->suspend_i (dispatch_info.handle_);
+
   // Release the lock.  Others threads can start waiting.
   guard.release_token ();
 
   int result = 0;
 
   // If there was an event handler ready, dispatch it.
-  if (dispatch_info.dispatch ())
+  /// Decrement the event left
+  --event_count;
+
+  if (this->dispatch_socket_event (dispatch_info) == 0)
+    ++result;                // Dispatched an event
+
+  int flag = 0;
+
+  // Acquire the token since we want to access the handler
+  // repository. The call to find () does not hold a lock and hence
+  // this is required.
+  guard.acquire_token ();
+
+  // Get the handler for the handle that we have dispatched to.
+  ACE_Event_Handler *eh =
+    this->handler_rep_.find (dispatch_info.handle_);
+
+  // This check is required for the following reasons
+  // 1. If dispatch operation returned a -1, then there is a
+  // possibility that the event handler could be deleted. In such
+  // cases the pointer to the event_handler that <dispatch_info>
+  // holds is set to 0.
+  //
+  // 2. If the application did its own memory management, a return
+  // value of 0 cannot be believed since the event handler could
+  // be deleted by the application based on some conditions. This
+  // is *bad*. But we dont have much of a choice with the existing
+  // reactor setup. To get around this, we can make a check for
+  // the handler registered with the repository for the handle
+  // that we have and compare with the handler that we
+  // posses. Yeah, I know it is like touching your nose by taking
+  // your hand around your head. But that is the way it is. This
+  // is a fix for [BUGID 1231]
+
+  if (dispatch_info.event_handler_ != 0 &&
+      eh == dispatch_info.event_handler_)
     {
-      /// Decrement the event left
-      --event_count;
-
-      if (this->dispatch_socket_event (dispatch_info) == 0)
-          ++result;                // Dispatched an event
-
-      int flag = 0;
-
-      // Hack of the decade ;-). We make an extra check for the handle
-      // in addition to the event handler before we make a check for
-      // the resume_handler ().
-      if (dispatch_info.event_handler_ != 0 &&
-          this->handler_rep_.find (dispatch_info.handle_) != 0)
-        {
-         flag =
+      flag =
            dispatch_info.event_handler_->resume_handler ();
-        }
-
-      if (dispatch_info.handle_ != ACE_INVALID_HANDLE &&
-          dispatch_info.event_handler_ != this->notify_handler_ &&
-          flag == 0)
-        this->resume_handler (dispatch_info.handle_);
     }
 
+  // Use resume_i () since we hold the token already.
+  if (dispatch_info.handle_ != ACE_INVALID_HANDLE &&
+      dispatch_info.event_handler_ != this->notify_handler_ &&
+      flag == ACE_Event_Handler::ACE_REACTOR_RESUMES_HANDLER)
+    this->resume_i (dispatch_info.handle_);
+
+  // Let me release the token here. This is not required since the
+  // destruction of the object on the stack will take care of this.
+  guard.release_token ();
+
   return result;
 }