ChangeLogTag:Wed Jan 30 17:22:49 2002 Steve Huston <shuston@riverace.com>

5 files changed, 157 insertions, 85 deletions

diff --git a/ChangeLog b/ChangeLog
index 99e2749a4a2..e5b73d5e87f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,26 @@
+Wed Jan 30 17:22:49 2002  Steve Huston  <shuston@riverace.com>
+
+        * ace/Process.cpp (wait (const ACE_Time_Value&, ACE_exitcode *)):
+        * ace/Process_Manager.cpp (wait (pid_t, const ACE_Time_Value&,
+                                         ACE_exitcode *)):
+          The mechanism for waiting up to a specified time for a child
+          process to exit has been replaced. Replaces the fix from:
+          Fri Jan 25 19:58:41 2002  Steve Huston  <shuston@riverace.com>
+          and makes unnecessary any further work from:
+          Sat Jan 26 21:41:39 2002  Steve Huston  <shuston@riverace.com>
+
+          Both classes now do a timed wait for a child by doing an
+          ACE_OS::sleep, counting on being interrupted if a SIGCHLD
+          is delivered. In ACE_Process_Manager when a reactor hasn't
+          been specified, and always in ACE_Process, a temporary
+          SIGCHLD handler is installed for the duration of the wait.
+          This is necessary because the default SIGCHLD action on
+          POSIX (and holds true for most non-Win32) is SIG_IGN, and
+          SIGCHLD is not generated when a child process exits.
+          Therefore, a handler is installed to force the SIGCHLD.
+          It's not needed in ACE_Process_Manager when a reactor is in
+          place because the reactor already has a handler for SIGCHLD.
+
 Wed Jan 30 15:11:49 2002  Krishnakumar B  <kitty@cs.wustl.edu>
 
         * include/makeinclude/platform_linux.GNU (CXX_VERSION):
diff --git a/ChangeLogs/ChangeLog-02a b/ChangeLogs/ChangeLog-02a
index 99e2749a4a2..e5b73d5e87f 100644
--- a/ChangeLogs/ChangeLog-02a
+++ b/ChangeLogs/ChangeLog-02a
@@ -1,3 +1,26 @@
+Wed Jan 30 17:22:49 2002  Steve Huston  <shuston@riverace.com>
+
+        * ace/Process.cpp (wait (const ACE_Time_Value&, ACE_exitcode *)):
+        * ace/Process_Manager.cpp (wait (pid_t, const ACE_Time_Value&,
+                                         ACE_exitcode *)):
+          The mechanism for waiting up to a specified time for a child
+          process to exit has been replaced. Replaces the fix from:
+          Fri Jan 25 19:58:41 2002  Steve Huston  <shuston@riverace.com>
+          and makes unnecessary any further work from:
+          Sat Jan 26 21:41:39 2002  Steve Huston  <shuston@riverace.com>
+
+          Both classes now do a timed wait for a child by doing an
+          ACE_OS::sleep, counting on being interrupted if a SIGCHLD
+          is delivered. In ACE_Process_Manager when a reactor hasn't
+          been specified, and always in ACE_Process, a temporary
+          SIGCHLD handler is installed for the duration of the wait.
+          This is necessary because the default SIGCHLD action on
+          POSIX (and holds true for most non-Win32) is SIG_IGN, and
+          SIGCHLD is not generated when a child process exits.
+          Therefore, a handler is installed to force the SIGCHLD.
+          It's not needed in ACE_Process_Manager when a reactor is in
+          place because the reactor already has a handler for SIGCHLD.
+
 Wed Jan 30 15:11:49 2002  Krishnakumar B  <kitty@cs.wustl.edu>
 
         * include/makeinclude/platform_linux.GNU (CXX_VERSION):
diff --git a/ChangeLogs/ChangeLog-03a b/ChangeLogs/ChangeLog-03a
index 99e2749a4a2..e5b73d5e87f 100644
--- a/ChangeLogs/ChangeLog-03a
+++ b/ChangeLogs/ChangeLog-03a
@@ -1,3 +1,26 @@
+Wed Jan 30 17:22:49 2002  Steve Huston  <shuston@riverace.com>
+
+        * ace/Process.cpp (wait (const ACE_Time_Value&, ACE_exitcode *)):
+        * ace/Process_Manager.cpp (wait (pid_t, const ACE_Time_Value&,
+                                         ACE_exitcode *)):
+          The mechanism for waiting up to a specified time for a child
+          process to exit has been replaced. Replaces the fix from:
+          Fri Jan 25 19:58:41 2002  Steve Huston  <shuston@riverace.com>
+          and makes unnecessary any further work from:
+          Sat Jan 26 21:41:39 2002  Steve Huston  <shuston@riverace.com>
+
+          Both classes now do a timed wait for a child by doing an
+          ACE_OS::sleep, counting on being interrupted if a SIGCHLD
+          is delivered. In ACE_Process_Manager when a reactor hasn't
+          been specified, and always in ACE_Process, a temporary
+          SIGCHLD handler is installed for the duration of the wait.
+          This is necessary because the default SIGCHLD action on
+          POSIX (and holds true for most non-Win32) is SIG_IGN, and
+          SIGCHLD is not generated when a child process exits.
+          Therefore, a handler is installed to force the SIGCHLD.
+          It's not needed in ACE_Process_Manager when a reactor is in
+          place because the reactor already has a handler for SIGCHLD.
+
 Wed Jan 30 15:11:49 2002  Krishnakumar B  <kitty@cs.wustl.edu>
 
         * include/makeinclude/platform_linux.GNU (CXX_VERSION):
diff --git a/ace/Process.cpp b/ace/Process.cpp
index 8e3a5831508..48ccdb91f57 100644
--- a/ace/Process.cpp
+++ b/ace/Process.cpp
@@ -13,6 +13,20 @@
 
 ACE_RCSID (ace, Process, "$Id$")
 
+
+// This function acts as a signal handler for SIGCHLD. We don't really want
+// to do anything with the signal - it's just needed to interrupt a sleep.
+// See wait() for more info.
+#if !defined (ACE_WIN32)
+static void
+sigchld_nop (int, siginfo_t *, ucontext_t *)
+{
+  return;
+}
+#endif /* ACE_WIN32 */
+
+
+
 ACE_Process::ACE_Process (void)
   :
 #if !defined (ACE_WIN32)
@@ -345,34 +359,40 @@ ACE_Process::wait (const ACE_Time_Value &tv,
   if (tv == ACE_Time_Value::max_time)
     return this->wait (status);
 
-  ACE_Time_Value wait_until = ACE_OS::gettimeofday () + tv;
-
-  for (;;)
+  // Need to wait but limited to specified time.
+  // Force generation of SIGCHLD, even though we don't want to
+  // catch it - just need it to interrupt the sleep below.
+  // If this object has a reactor set, assume it was given at
+  // open(), and there's already a SIGCHLD action set, so no
+  // action is needed here.
+  ACE_Sig_Action old_action;
+  ACE_Sig_Action do_sigchld ((ACE_SignalHandler)sigchld_nop);
+  do_sigchld.register_action (SIGCHLD, &old_action);
+
+  pid_t pid;
+  ACE_Time_Value tmo (tv);       // Need one we can change
+  for (ACE_Countdown_Time time_left (&tmo); ; time_left.update ())
     {
-      int result = ACE_OS::waitpid (this->getpid (),
-                                    status,
-                                    WNOHANG);
-      if (result != 0)
-        return result;
-
-      ACE_Sig_Set alarm_or_child;
-
-      alarm_or_child.sig_add (SIGALRM);
-      alarm_or_child.sig_add (SIGCHLD);
-
-      ACE_Time_Value time_left = wait_until - ACE_OS::gettimeofday ();
-
-      // If ACE_OS::ualarm doesn't have sub-second resolution:
-      time_left += ACE_Time_Value (0, 500000);
-      time_left.usec (0);
+      pid = ACE_OS::waitpid (this->getpid (), status, WNOHANG);
+      if (pid > 0 || pid == ACE_INVALID_PID)
+        break;          // Got a child or an error - all done
+
+      // pid 0, nothing is ready yet, so wait.
+      // Do a sleep (only this thread sleeps) til something
+      // happens. This relies on SIGCHLD interrupting the sleep.
+      // If SIGCHLD isn't delivered, we'll need to do something
+      // with sigaction to force it.
+      if (-1 == ACE_OS::sleep (tmo) && errno == EINTR)
+        continue;
+      // Timed out
+      pid = 0;
+      break;
+    }
 
-      if (time_left <= ACE_Time_Value::zero)
-        return 0; // timeout
+  // Restore the previous SIGCHLD action if it was changed.
+  old_action.register_action (SIGCHLD);
 
-      ACE_OS::ualarm (time_left);
-      if (ACE_OS::sigwait (alarm_or_child) == -1)
-        return ACE_INVALID_PID;
-    }
+  return pid;
 #endif /* ACE_WIN32 */
 }
 
diff --git a/ace/Process_Manager.cpp b/ace/Process_Manager.cpp
index 043a33a369d..b59a1da78f0 100644
--- a/ace/Process_Manager.cpp
+++ b/ace/Process_Manager.cpp
@@ -28,6 +28,18 @@ ACE_Process_Manager::cleanup (void *, void *)
   ACE_Process_Manager::close_singleton ();
 }
 
+// This function acts as a signal handler for SIGCHLD. We don't really want
+// to do anything with the signal - it's just needed to interrupt a sleep.
+// See wait() for more info.
+#if !defined (ACE_WIN32)
+static void
+sigchld_nop (int, siginfo_t *, ucontext_t *)
+{
+  return;
+}
+#endif /* ACE_WIN32 */
+
+
 ACE_ALLOC_HOOK_DEFINE(ACE_Process_Manager)
 
 // Singleton instance.
@@ -806,70 +818,41 @@ ACE_Process_Manager::wait (pid_t pid,
         }
       else
         {
-          for (;;)
+          // Force generation of SIGCHLD, even though we don't want to
+          // catch it - just need it to interrupt the sleep below.
+          // If this object has a reactor set, assume it was given at
+          // open(), and there's already a SIGCHLD action set, so no
+          // action is needed here.
+          ACE_Sig_Action old_action;
+          if (this->reactor () == 0)
+            {
+              ACE_Sig_Action do_sigchld ((ACE_SignalHandler)sigchld_nop);
+              do_sigchld.register_action (SIGCHLD, &old_action);
+            }
+
+          ACE_Time_Value tmo (timeout);  // Need one we can change
+          for (ACE_Countdown_Time time_left (&tmo); ; time_left.update ())
             {
               pid = ACE_OS::waitpid (-1, status, WNOHANG);
-              if (pid > 0)
-                break;          // Got one - all done
-              if (pid == -1)
-                {
-                  if (errno == EINTR)
-                    continue;   // Try again
-                  break;        // Real error - give up
-                }
-              // pid 0, nothing is ready yet, so wait.
+              if (pid > 0 || pid == ACE_INVALID_PID)
+                break;          // Got a child or an error - all done
 
-              ACE_Sig_Set wait_for_sigs;
-
-              wait_for_sigs.sig_add (SIGCHLD);
-
-#  if defined (ACE_HAS_SIGTIMEDWAIT)
-              // Block SIGCHLD then wait for it.
-              sigset_t orig_set;
-#    if defined (ACE_HAS_THREADS)
-              ACE_OS::thr_sigsetmask (SIG_BLOCK, wait_for_sigs, &orig_set);
-#    else
-              ACE_OS::sigprocmask (SIG_BLOCK, wait_for_sigs, &orig_set);
-#    endif
-              int status = ACE_OS::sigtimedwait (wait_for_sigs,
-                                                 0,
-                                                 &timeout);
-              {
-                ACE_Errno_Guard guard (errno);
-#    if defined (ACE_HAS_THREADS)
-                ACE_OS::thr_sigsetmask (SIG_BLOCK, wait_for_sigs, &orig_set);
-#    else
-                ACE_OS::sigprocmask (SIG_SETMASK, &orig_set, 0);
-#    endif
-              }
-              if (-1 != status || errno == EINTR)
-                continue;       // SIGCHLD ready; go waitpid again
-              // Here if there was a timeout or error. If timeout,
-              // it's not an error for our caller, just return 0.
-              pid = (errno == EAGAIN ? 0 : ACE_INVALID_PID);
+              // pid 0, nothing is ready yet, so wait.
+              // Do a sleep (only this thread sleeps) til something
+              // happens. This relies on SIGCHLD interrupting the sleep.
+              // If SIGCHLD isn't delivered, we'll need to do something
+              // with sigaction to force it.
+              if (-1 == ACE_OS::sleep (tmo) && errno == EINTR)
+                continue;
+              // Timed out
+              pid = 0;
               break;
-#  else
-              // Without sigtimedwait support, this can get hairy... we
-              // set an alarm to fire at the caller-specified future time.
-              // Then we wait for either SIGCHLD or SIGALRM. The problem with
-              // this is that once ualarm is enabled, the SIGALRM will fire
-              // whether we're still here waiting or not. On some platforms,
-              // this will cause a core dump on uncaught signal.
-              ACE_Time_Value time_left (timeout);
-
-              // if ACE_OS::ualarm doesn't have sub-second resolution:
-              time_left += ACE_Time_Value (0, 500000);
-              time_left.usec (0);
-
-              if (time_left <= ACE_Time_Value::zero) {
-                pid = 0;
-                break;
-              }
-
-              wait_for_sigs.sig_add (SIGALRM);
-              ACE_OS::ualarm (time_left);
-              ACE_OS::sigwait (wait_for_sigs);
-#  endif /* ACE_HAS_SIGTIMEDWAIT */
+            }
+
+          // Restore the previous SIGCHLD action if it was changed.
+          if (this->reactor () == 0)
+            {
+              old_action.register_action (SIGCHLD);
             }
         }
 #endif /* !defined (ACE_WIN32) */