Re: Proposed Patch for Zombies Left by Dpi_start_dpid() in dpi.c

Hi Johannes, JG:

On Thu, Aug 29, 2013 at 09:13:49AM +0200, Johannes Hofmann wrote:
> Hi Jorge,
> 
> On Wed, Aug 28, 2013 at 08:48:35PM -0400, Jorge Arellano Cid wrote:
> > Hi,
> > 
> > On Wed, Aug 28, 2013 at 07:51:34PM +0200, Johannes Hofmann wrote:
> > > Hi JG,
> > > 
> > > On Tue, Aug 27, 2013 at 03:15:14PM -0400, JG wrote:
> > > > It seems that the dpid children forked by Dpi_start_dpid() in dpi.c
> > > > are not reaped when they terminate themselves after a period of
> > > > inactivity while the parent dillo process is still running.  The patch
> > > > below is simply to reap these zombies.  The SIGCHLD handler should
> > > > probably do more, but I am not yet familiar enough with Dillo to know
> > > > what.
> > > > 
> > > > I believe this patch adheres to the Dillo rules of coding style, but
> > > > am new to the list and a novice at Dillo patching.  And hence also
> > > > apologies to Jorge for this duplication of a previous communication by
> > > > private email (although with one small change to get the patch to
> > > > compile under OpenBSD as well as Linux).
> > > 
> > > I can reproduce the issue here by killing dpid manually.
> > > For me the following also fixes it:
> > > 
> > > diff -r 659dc205c377 src/dillo.cc
> > > --- a/src/dillo.cc    Wed Aug 21 11:42:38 2013 +0200
> > > +++ b/src/dillo.cc    Wed Aug 28 19:48:10 2013 +0200
> > > @@ -329,6 +329,7 @@
> > >  
> > >     // Some OSes exit dillo without this (not GNU/Linux).
> > >     signal(SIGPIPE, SIG_IGN);
> > > +   signal(SIGCHLD, SIG_IGN);
> > >  
> > >     /* Handle command line options */
> > >     opt_argv = dNew0(char*, numOptions(Options) + 1);
> > > 
> > > Can you please check whether it works for you too? It would have the
> > > advantage that we have all signal related stuff in one place.
> > 
> >   Oh,  I'm looking at it now too! 
> > For me this is an old TODO: with subtle issues lurking.
> > 
> > 
> >   Somehow, setting SIG_IGN to a child rings me wrong...  :-)
> > 
> > 
> >   After some digging (Manual page sigaction(2)):
> > 
> > ...
> > 
> > "A  child  created  via  fork(2)  inherits a copy of its parent's
> > signal  dispositions.  During  an  execve(2), the dispositions of
> > handled  signals  are  reset  to the default; the dispositions of
> > ignored signals are left unchanged."
> 
> I guess this is no problem, as dpid explicitely adds a signal
> handler for SIGCHLD again, but I'm happy if you take care of the
> whole issue :-)

  The attached program is a stress test for SIGCHLD. The idea is to
find a formula that works well even in such cases. IIRC both of you
have access to BSD machines so please test it there.

  The  program  spawns  255  children  which  sleep 5 seconds and
terminate generating a SIGCHLD avalanche, which should be handled
cleanly. It works OK in Debian.

    $gcc -W -Wall sigcld.c -o sigcld
    $./sigcld

  in another terminal (during the 5 secs. sleep window):

    $ps aux|grep sigcld

    (shows the long list)

  then while main is Sleeping 10 seconds:

    $ps aux|grep sigcld

    (shows only one sigcld process)

  after it exits:

    $ps aux|grep sigcld

    (no process left)

  TIA.

-- 
  Cheers
  Jorge.-

_______________________________________________
Dillo-dev mailing list
[email protected]
http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

#include <sys/types.h>
#include <sys/wait.h>
#include <wait.h>
#include <unistd.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <errno.h>

// SIGCHLD -------------------------------------------------------------------
static int gone = 0;


/*
 * Avoid dpid becoming a zombie.
 * Notes:
 *   . We let sigaction block SIGCHLD while in the handler.
 *   . Portability is not simple. e.g.
 *       http://www.faqs.org/faqs/unix-faq/faq/part3/section-13.html
 *       man sigaction, waitpid
 */
static void raw_sigchld2(int signum)
{
   pid_t pid;
   int status;

   while (1) {
      pid = waitpid(-1, &status, WNOHANG);
      if (pid > 0) {
         if (WIFEXITED(status))        /* normal exit */
            printf("e%d ", WEXITSTATUS(status));
         else if (WIFSIGNALED(status)) /* terminated by signal */
            printf("s%d ", WTERMSIG(status));
         ++gone;
      } else if (pid == 0 || errno == ECHILD) {
         break;
      } else {
         if (errno == EINTR)
            continue;
         perror("waitpid");
         break;
      }
   }
   ++signum; /* compiler happiness */
}

/*! Establish SIGCHLD handler */
static void est_sigchld(void)
{
   struct sigaction sigact;
   sigset_t set;

   (void) sigemptyset(&set);
   sigact.sa_handler = raw_sigchld2; /* our custom handler */
   sigact.sa_mask = set;             /* no aditional signal blocks */
   sigact.sa_flags = SA_NOCLDSTOP;   /* ignore stop/resume states */
   if (sigaction(SIGCHLD, &sigact, NULL) == -1) {
      perror("sigaction");
      exit(1);
   }
}



// Main ----------------------------------------------------------------------

int main(void)
{
   int i;
   pid_t child;

   /* Install our SIGCHLD handler */
   est_sigchld();

   for (i = 0; i < 256; ++i) {
      if ((child = fork()) < 0) {
         perror("fork");
         exit(-1);
      } else if (child == 0) {
         /* in child */
         sleep(5);
         break;
      } else {
         /* in parent */
         printf("num children = %d\n", i);
      }
   }
 
   if (child == 0) {
      /* in child */
      return 0;
   }
 
   /* In parent */
   while (gone < 256) {
      printf("%d ", gone);
      sleep(1);
   }

   printf("Sleeping 10 sec...\n");
   sleep(10);
   return 0;
}