Revision 1e97f0047cd3e2b4639f29469969dc699089ba1c

[Mark Doliner]

On Mon, 26 Feb 2007 06:21:30 -0500, Evan Schoenberg wrote
> On Feb 25, 2007, at 10:58 PM, Mark Doliner wrote:
> 
> > On Sun, 25 Feb 2007 18:51:57 -0500 (EST), evands wrote
> >> -----------------------------------------------------------------
> >> Revision: 1e97f0047cd3e2b4639f29469969dc699089ba1c
> >> Ancestor: 238117a5836fa4e00ce2917d0ac5878906ebb38f
> >> Author: evands at pidgin.im
> >> Date: 2007-02-25T23:50:54
> >> Branch: im.pidgin.pidgin
> >>
> >> Modified files:
> >>         libpurple/dnssrv.c
> >>
> >> ChangeLog:
> >>
> >> Patch from Graham Booker which ensures that a process forked by
> >> dnsserv cleans up afterwards, calling waitpid() to make sure no
> >> zombies are left over.
> >
> > I thought we had code in pidgin/gtkmain.c that took care of that?   
> > I think we
> > have a signal set up that calls a function anytime a child process  
> > dies.  This
> > change seems superfluous to me within Pidgin.  Is it needed in  
> > Adium or
> > Pidgin-text?
> 
> There is indeed code in gtkmain.c which makes this technically not  
> needed; it's the clean_pid() function.
> 
> There is currently a similar function in Adium's implementation...  
> which was added only after several versions were released in which  
> there was an extremely mysterious growth over time of zombie  
> processes.  I'm not aware of any documentation stating, "Gaim's  
> implementation of child processes is such that code elsewhere must 
> be  careful to clean up after them using a zombie reaper."
> 
> Good contract-based programming practices, in my humble opinion,  
> demand that code should be locally independent wherever possible, by 
>  which I mean that if at all feasible a given module should not 
> create  demands of code elsewhere, especially as regards 
> implementation- specific details (a UI developer should not need to 
> go look at the  source of each file, see if there are any fork() 
> uses which don't  clean up after themselves, then account for it).
> 
> The change in question means that, in the case of dnssrv, no 
> clean_pid 
> () style function is necessary for the fork() based implementation;  
> if there's a zombie, dnssrv itself takes care of it, and if there's  
> not, there's no harm done (since waitpid() returns immediately).  In 
>  the case of Pidgin, clean_pid() is still needed because apparently  
> GStreamer isn't as polite as I'd like libpurple to be, and cleaning  
> up after forked'd processes is needed.  In the case of Adium, this 
>  change now means that every library we're using at present is self- 
> contained, and no such function is needed.
> 
> Of course, when we go all voice-video, we'll be using GStreamer,  
> too... (I hadn't known until just now when I looked at gtkmain.c 
> that  there was a GStreamer related problem with such things).  This 
> means  that we'll end up needed a clean_pid(), as well... but that 
> doesn't  change my original claim: it's just good practices for code 
> to be  self-contained.
> 
> Thoughts?

Ah, I see.  All very good points.  I guess it'd be good if we changed the
Pidgin code so that it was more clear that the child process reaping code is
now only needed for GStreamer.  Or maybe even find a way to move it into
gtksound.c.

-Mark