GNOME Bugzilla – Bug 740929
Leaves zombie shell when closing with X
Last modified: 2014-12-04 21:01:05 UTC
Found at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=770596 When closing a tab or window with the X button or with Ctrl+Shift+W or alike (as opposed to asking the shell to exit), a defunct bash is left behind. These are not cleaned up until you exit from g-t, which might take (depending on someone's workflow) days/weeks, and easily result in hundreds of zombies cluttering the output of ps and consuming the scarce resource of 32k possible process IDs.
Probably relevant: bug 112172 https://git.gnome.org/browse/vte/commit/?id=de112fd
Hmm. vte_terminal_finalize first removes the child watch source, then kill()s the process. So the child won't be reaped. But I'm not sure inverting the order of these calls will fix it.
Created attachment 291842 [details] [review] Fix Here's my first attempt to fix it (I hope it's good). Sending a SIGHUP to the child is necessary (we need to notify the shell), but I don't think shuffling the order would solve anything: - We shouldn't leave zombies even if the child ignores sighup (maybe is not even a shell, but some random app) - There's a race condition, we can't be sure when the shell will actually exit. My current attempt is to install another child_watch handler, one that does nothing when executed. Not sure if the child_watch source cleans up itself or if I leave around some stuff in Glib, but it's already better than zombies.
Review of attachment 291842 [details] [review]: A child watch source is always removed after the callback is called. See g_child_watch_dispatch in GLib. That is why GChildWatchFunc does not need to return a boolean unlike a GSourceFunc. I am not a vte reviewer, though.
Created attachment 291877 [details] [review] Fix v2 Thanks Debarshi, based on this I'm even more positive that this is the right fix. Update: cosmetic changes only.
Or we could re-introduce the reaper but keep it private not exported.
As far as I understand, the reaper was removed for good reasons. At this point I'd like to go for the easiest possible fix, and cherry-pick to stable. I'm not against further refactoring to make it more proper OO, maybe even bringing back a better reaper, but IMO such change should go in master only, not in stable branches. I'd like to commit my change if you're not against it, and then let you refactor it if you wish :)
Sure, please commit. The reason the reaper was removed was that we wanted it gone from the public API, and I didn't realise this subtle problem exists.
I just realized there were 8 years between the initial bugreport and the removal of the reaper. Initial concerns (e.g. manually hijacking sigchld) didn't exist by the time it was removed. So it's okay to bring it back, but of not much use.
Comment on attachment 291877 [details] [review] Fix v2 Committed to master. Will backport to stable in a few days if no problem arises.
Fixed in 0-38 too.
Review of attachment 291877 [details] [review]: ::: src/vte.c @@ +8460,3 @@ + terminal->pvt->pty_pid, + (GChildWatchFunc)vte_terminal_child_watch_cb, + NULL, NULL); This needs to go inside the if block. Otherwise, when we are closing the terminal by actually quitting the shell, pvt->pty_pid would be -1 (and pvt->child_watch_source would be 0), which lead to a CRITICAL from g_child_watch_add_full.
Created attachment 292134 [details] [review] widget: Reap only when a child is present
Comment on attachment 292134 [details] [review] widget: Reap only when a child is present Oops, indeed, thanks a lot for this hotfix! Committed.