ChangeLogTag:Thu Sep 6 18:51:47 UTC 2012 Steve Huston <shuston@riverace.com>

8 files changed, 84 insertions, 27 deletions

diff --git a/ACE/ChangeLog b/ACE/ChangeLog
index bc51a288c32..9f10a5168c8 100644
--- a/ACE/ChangeLog
+++ b/ACE/ChangeLog
@@ -1,3 +1,25 @@
+Thu Sep  6 18:51:47 UTC 2012  Steve Huston  <shuston@riverace.com>
+
+        * ace/Sig_Handler.h:
+        * ace/Sig_Handler.cpp: Call back to ACE_Event_Handler::handle_close()
+          when a signal handler is removed by any means. The callback was
+          previously made only when dispatched handle_signal() returned -1.
+          This adds the callback when the signal handler is unregistered
+          either explicitly or by a containing reactor closing. Resolves
+          Bugzilla #2368.
+
+        * ace/Process_Manager.h:
+        * ace/Process_Manager.cpp: Added a handle_close() method to be notified
+          when a reactor with which SIGCHLD is registered closes, so no
+          attempt is later made to remove the SIGCHLD registration.
+
+        * NEWS: Describe the above enhancement.
+
+        * tests/Bug_2368_Regression_Test.cpp: Register the two handlers for
+          different signals so they're both tracked and removed properly.
+
+        * tests/run_test.lst: Enable Bug_2368_Regression_Test.
+
 Thu Sep  6 15:06:15 UTC 2012  Steve Huston  <shuston@riverace.com>
 
         * ace/Log_Msg.cpp (close): Don't try to lock/unlock the
diff --git a/ACE/NEWS b/ACE/NEWS
index e6a34c54594..b3de032e543 100644
--- a/ACE/NEWS
+++ b/ACE/NEWS
@@ -1,6 +1,13 @@
 USER VISIBLE CHANGES BETWEEN ACE-6.1.4 and ACE-6.1.5
 ====================================================
 
+. When a ACE_Event_Handler registered for signals is unregistered,
+  whether by unregistering, returning -1 from handle_signal(), or by
+  the reactor closing, the ACE_Event_Handler::handle_close() hook will
+  be called. The close_mask passed will be ACE_Event_Handler::SIGNAL_MASK.
+  In previous versions, handle_close() would only be called when the
+  handle_signal() callback returned -1. This resolves Bugzilla #2368.
+
 USER VISIBLE CHANGES BETWEEN ACE-6.1.3 and ACE-6.1.4
 ====================================================
 
diff --git a/ACE/ace/Process_Manager.cpp b/ACE/ace/Process_Manager.cpp
index 7cec0702d68..1884bdadc7b 100644
--- a/ACE/ace/Process_Manager.cpp
+++ b/ACE/ace/Process_Manager.cpp
@@ -329,6 +329,19 @@ ACE_Process_Manager::handle_input (ACE_HANDLE)
   return 0;
 }
 
+int
+ACE_Process_Manager::handle_close (ACE_HANDLE /* handle */,
+                                   ACE_Reactor_Mask close_mask)
+{
+  ACE_TRACE ("ACE_Process_Manager::handle_close");
+  if (close_mask == ACE_Event_Handler::SIGNAL_MASK)
+    {
+      // Reactor is telling us we're gone; don't unregister again later.
+      this->reactor (0);
+    }
+  return 0;
+}
+
 #endif /* !ACE_WIN32 */
 
 // On Unix, this routine is called asynchronously when a SIGCHLD is
diff --git a/ACE/ace/Process_Manager.h b/ACE/ace/Process_Manager.h
index 4d4496b6a59..51e5ba95eb8 100644
--- a/ACE/ace/Process_Manager.h
+++ b/ACE/ace/Process_Manager.h
@@ -354,6 +354,12 @@ protected:
 #if !defined(ACE_WIN32)
   /// Collect one (or more, on unix) process exit status.
   virtual int handle_input (ACE_HANDLE proc);
+
+  /// If registered with a reactor for SIGCHLD and the reactor closes, this
+  /// will get called to notify.
+  virtual int handle_close (ACE_HANDLE handle,
+                            ACE_Reactor_Mask close_mask);
+
 #endif // !defined(ACE_WIN32)
 
   /**
diff --git a/ACE/ace/Sig_Handler.cpp b/ACE/ace/Sig_Handler.cpp
index d408b8ba839..fdc8531bbdd 100644
--- a/ACE/ace/Sig_Handler.cpp
+++ b/ACE/ace/Sig_Handler.cpp
@@ -54,6 +54,9 @@ ACE_ALLOC_HOOK_DEFINE(ACE_Sig_Handler)
 
 ACE_Sig_Handler::~ACE_Sig_Handler (void)
 {
+  for (int s = 1; s < ACE_NSIG; ++s)
+    if (ACE_Sig_Handler::signal_handlers_[s])
+      ACE_Sig_Handler::remove_handler_i (s);
 }
 
 void
@@ -194,6 +197,30 @@ ACE_Sig_Handler::register_handler (int signum,
                                               old_disp);
 }
 
+int
+ACE_Sig_Handler::remove_handler_i (int signum,
+                                   ACE_Sig_Action *new_disp,
+                                   ACE_Sig_Action *old_disp,
+                                   int)
+{
+  ACE_TRACE ("ACE_Sig_Handler::remove_handler_i");
+
+  ACE_Sig_Action sa (SIG_DFL, (sigset_t *) 0); // Reset to default disposition.
+
+  if (new_disp == 0)
+    new_disp = &sa;
+
+  ACE_Event_Handler *eh = ACE_Sig_Handler::signal_handlers_[signum];
+  ACE_Sig_Handler::signal_handlers_[signum] = 0;
+
+  // Allow the event handler to close down if necessary.
+  eh->handle_close (ACE_INVALID_HANDLE,
+                    ACE_Event_Handler::SIGNAL_MASK);
+
+  // Register either the new disposition or restore the default.
+  return new_disp->register_action (signum, old_disp);
+}
+
 // Remove an ACE_Event_Handler.
 
 int
@@ -209,17 +236,7 @@ ACE_Sig_Handler::remove_handler (int signum,
     ACE_GUARD_RETURN (ACE_Recursive_Thread_Mutex, m, *lock, -1));
 
   if (ACE_Sig_Handler::in_range (signum))
-    {
-      ACE_Sig_Action sa (SIG_DFL, (sigset_t *) 0); // Define the default disposition.
-
-      if (new_disp == 0)
-        new_disp = &sa;
-
-      ACE_Sig_Handler::signal_handlers_[signum] = 0;
-
-      // Register either the new disposition or restore the default.
-      return new_disp->register_action (signum, old_disp);
-    }
+    return ACE_Sig_Handler::remove_handler_i (signum, new_disp, old_disp);
 
   return -1;
 }
@@ -249,20 +266,7 @@ ACE_Sig_Handler::dispatch (int signum,
   if (eh != 0)
     {
       if (eh->handle_signal (signum, siginfo, ucontext) == -1)
-        {
-          // Define the default disposition.
-          ACE_Sig_Action sa ((ACE_SignalHandler) SIG_DFL, (sigset_t *) 0);
-
-          ACE_Sig_Handler::signal_handlers_[signum] = 0;
-
-          // Remove the current disposition by registering the default
-          // disposition.
-          sa.register_action (signum);
-
-          // Allow the event handler to close down if necessary.
-          eh->handle_close (ACE_INVALID_HANDLE,
-                            ACE_Event_Handler::SIGNAL_MASK);
-        }
+        ACE_Sig_Handler::remove_handler_i (signum);
 #if defined (ACE_WIN32)
       else
         // Win32 is weird in the sense that it resets the signal
diff --git a/ACE/ace/Sig_Handler.h b/ACE/ace/Sig_Handler.h
index 3e7e80891d3..3e8732e5579 100644
--- a/ACE/ace/Sig_Handler.h
+++ b/ACE/ace/Sig_Handler.h
@@ -131,6 +131,11 @@ protected:
                                  ACE_Event_Handler **old_sh = 0,
                                  ACE_Sig_Action *old_disp = 0);
 
+  static int remove_handler_i (int signum,
+                               ACE_Sig_Action *new_disp = 0,
+                               ACE_Sig_Action *old_disp = 0,
+                               int sigkey = -1);
+
   /// Check whether the SIGNUM is within the legal range of signals.
   static int in_range (int signum);
 
diff --git a/ACE/tests/Bug_2368_Regression_Test.cpp b/ACE/tests/Bug_2368_Regression_Test.cpp
index 3606c24e8f1..242957533ba 100644
--- a/ACE/tests/Bug_2368_Regression_Test.cpp
+++ b/ACE/tests/Bug_2368_Regression_Test.cpp
@@ -84,7 +84,7 @@ run_main (int, ACE_TCHAR *[])
                       -1);
 
   if (ACE_Reactor::instance ()->register_handler
-      (SIGINT,
+      (SIGCHLD,
        &my_handlerB) == -1)
     ACE_ERROR_RETURN ((LM_DEBUG,
                        "%p\n",
diff --git a/ACE/tests/run_test.lst b/ACE/tests/run_test.lst
index fbc4db27b39..3c4a7a74e14 100644
--- a/ACE/tests/run_test.lst
+++ b/ACE/tests/run_test.lst
@@ -32,7 +32,7 @@ Bound_Ptr_Test: !ACE_FOR_TAO
 Buffer_Stream_Test
 Bug_1576_Regression_Test
 Bug_1890_Regression_Test
-Bug_2368_Regression_Test: ALL !DISABLED
+Bug_2368_Regression_Test:
 Bug_2434_Regression_Test
 Bug_2497_Regression_Test
 Bug_2540_Regression_Test