Subject: bug#14474: 24.3.50; Zombie subprocesses (again)

From: Jan Djärv <[email-masked]>
Date: Tue, 28 May 2013 19:04:59 +0200

Hello.

27 maj 2013 kl. 19:36 skrev Paul Eggert <[email-masked]>:

> [The context is
> http://bugs.gnu.org/14474
> ]
> 
> On 05/27/2013 05:46 AM, Colin Walters wrote:
> 
>> Basically it's going to be very hard over time to avoid codepaths
>> in the GTK+ stack that don't call g_spawn_*() indirectly, thus
>> installing a SIGCHLD handler
> 
> Thanks.  In that case, shouldn't the glib documentation be
> changed to warn application developers not to install a SIGCHLD
> handler as well?  Currently it warns them only to not call
> waitpid(-1, ...).
> 
> Are application developers allowed to temporarily mask SIGCHLD?
> Emacs does that a lot.
> 
>>> One possibility is to see if we can get Emacs to use
>>>> glib's child watcher.
>> That'd be best obviously.
> 

> I suspect so too, but it requires more expertise in
> glib than I have (which is, basically, nothing).
> If I understand things correctly, if Emacs is using
> Gtk it should
> 

Actually GLib is linked in whenever one of GSettings, GConf, Gtk or rsvg is used.
I see rsvg only is not handeled in xgselect.c, an oversight.

> * never call sigaction (SIGCHLD, ...) or signal (SIGCHLD, ...)
>   or waitpid (-1, ...).
>   E.g., remove the current call to sigaction (SIGCHLD, ...),
>   in src/process.c's init_process_emacs.
> 
> * Whenever Emacs creates a child process, use the
>   following pattern:
> 
>       block SIGCHLD;
>       pid = vfork ();
>       if (pid > 0)
>         {
>           record pid in Emacs's process table, as location 'loc';
>           record in *loc that glib is watching this pid;
>           g_child_watch_add (pid, watcher, loc);
>         }
>       unblock SIGCHLD;
> 
>  * never call waitpid (pid, ...) if PID is recorded
>    in Emacs's process table as something that glib is
>    watching.
> 
>  * Add a glue function ("watcher", above) that does
>    something like this:
> 
>      void watcher (GPid pid, gint status, gpointer loc) {
> 	block SIGCHLD
>        record that PID exited with status STATUS, by modifying *LOC,
> 	  sort of like's what currently done in handle_child_signal;
>        if (input_available_clear_time)
> 	  *input_available_clear_time = make_emacs_time (0, 0);
>        unblock SIGCHLD
>     }
> 
> But this sounds incomplete.  No doubt there's something
> about the main loop, or setting up the watchers, that I don't
> know about.  E.g., how does one remove the watcher once it
> has fired and told us that the process has exited?
> 

Keep track of the return value from g_child_watch_add and pass it to g_source_remove.
I think g_source_remove can be called in the callback function.

We kind of use GLibs main loop in xgselect.c, so child watches should be called from there.
As GLib:s main loop is an "all or nothing" approach, we could also move the filedescriptor and timeout handling  there.  Then xgselect.c could more or less go away.  But there is no real gain to do that, xgselect works well enough.

	Jan D.