Re: GChildWatchSource take three

[Owen Taylor <otaylor redhat com>]

On Tue, 2003-09-16 at 01:06, Jonathan Blandford wrote:
> Owen Taylor <otaylor redhat com> writes:
> 
> > On Wed, 2003-07-30 at 14:16, Jonathan Blandford wrote:
> > > Hi Owen,
> > > 
> > > Here's the latest version of my patch.  I added more to the docs as well
> > > as fixing other things discussed with Alex.  The big question I have is
> > > how best to handle ECHILD situations.  Currently, I'm passing -1 as the
> > > status.  As an alternative, I can just not dispatch those sources as
> > > well.
> > 
> > Commented on below with a few other things; despite the length of
> > the comments, it looks basically pretty solid; most of this stuff
> > is details, and the stuff that isn't details shouldn't be hard
> > to fix.
> 
> I made most of the changes you suggested.  There were two I had problems
> with.  Also, I'm assuming this won't work with win32.  Should I go ahead
> and add #ifdef G_OS_UNIX guards to gmain.[ch]?
> 
> Some comments on the comments.
> > ====
> > +void
> > +_g_main_thread_init ()
> > +{
> > +  add_pipes_to_main_contexts ();
> > +}
> > ====
> > 
> > Don't you need to do the SINGLE => THREADED initialization
> > here? If you don't do it here, then a child watched
> > added before g_thread_init() won't fire correctly 
> > after it.
> 
> I don't do SINGLE => THREADED initialization here as we may not want to
> use the child_watch stuff.  

If you are in SINGLE, you are using the child_watch stuff, no?

> > ECHILD indicates a programmatic error, and you should g_warning()
> > and probably avoid dispatching. Anyways, I believe a status of -1 
> > could be a legitimate exit status, so you can't use it as a flag value.
> 
> I wonder if there are ever legitimate reasons to want to get ECHILD?

If you don't know that there is a child (possibly zombie) with a given
PID that you created, then you have no business passing that ID
to waitpid.

> > This doens't work on several aspects:
> > 
> >  A) g_main_context_wakeup_unlocked() is a noop
> >     ifndef G_THREADS_ENABLED
> > 
> >  B) g_main_context_wakeup_unlocked() checks
> >      if (g_thread_supported() && context->poll_waiting)
> >     so it's a no-op when you call it here in the
> >     single threaded case.
> > 
> >  C) g_main_context_wakeup_unlocked() isn't signal safe. In the
> >     sequence
> > 
> > ===
> > ifdef G_THREADS_ENABLED
> >   if (!context->poll_waiting)         <= A
> >     {
> > #ifndef G_OS_WIN32
> >       gchar c;
> >       read (context->wake_up_pipe[0], &c, 1);
> > #endif
> >     }
> >   else
> >     context->poll_waiting = FALSE;   <= B
> > ===
> >     
> >    if your signal handler runs between A and B, you
> >    leak a byte into the pipe, and your main loop will
> >    spin at 100% cpu because it gets left there. I think you 
> >    can fix this by doing:
> > 
> >   if (context->wake_up_rec.revents & G_IO_IN)
> >     {
> >       /* Read a bunch of bytes from the pipe */
> >     }
> >   context->poll_waiting = FALSE;
> > 
> >   And then use a custom wake up function in your
> >   signal handler that doesn't look at g_thread_supported().
> 
> I added the custom wake up function, but I'm not sure what you were
> proposing for g_main_context_check.  I tried replacing the 
> (!context->poll_waiting) test but that seemed to break timeouts.

I don't think it should. I'm not really sure how to explain
further ... context->wake_up_rec.revents should have G_IO_IN if there
is stuff waiting to read out of the wake-up pipe.

Regards,
					Owen