This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8497

procutil: always waiting on child processes to prevent zombies with 'hg serve'
ClosedPublic

Authored by rdamazio on May 7 2020, 6:18 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGed684a82e29b: procutil: always waiting on child processes to prevent zombies with 'hg serve'
rHG38e0c006b0ae: procutil: always waiting on child processes to prevent zombies with 'hg serve'
Summary

When runbgcommand is invoked by an extension with ensurestart=False, we never
called waitpid - which is fine in most cases, except if that's happening on a
command server (e.g. chg), in which case the child defunct process will just
sit there for as long as the server is running.

The actual semantics of SIGCHLD signal handling is a lot more complex than
it seems, and the POSIX standard *seems* to read that it's ignored by default
and everything would just work without the waitpid if we're not listening for
it, but the truth is that it's only ignored if we *explicitly* set it to
SIG_IGN. We further cannot set it to SIG_IGN or to a catch-all handler across
all of 'hg serve', because Python's suprocess.Popen relies on that signal,
and a few specific parts of hg also set custom handlers, so instead we wait
for specific PIDs in dedicated threads.

I did a poor-man's benchmark of the thread creation and it seems to take
about 1ms, which is way better than the 20+ms from ensurestart=True.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rdamazio created this revision.May 7 2020, 6:18 AM
rdamazio added a commit: rHG38e0c006b0ae: procutil: always waiting on child processes to prevent zombies with 'hg serve'.May 7 2020, 11:02 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG38e0c006b0ae: procutil: always waiting on child processes to prevent zombies with 'hg serve' (authored by rdamazio). · Explain Why
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.May 7 2020, 11:40 AM
martinvonz added inline comments.
hg
1

I'll update the queued commit to not have this change

Revision Contents
Changeset List

PathPackages
Mhg (2 lines)
Mmercurial/utils/procutil.py (9 lines)

Diff 21281

hg

#!/usr/bin/env python #!/usr/bin/env python3
martinvonzUnsubmitted
Not Done

I'll update the queued commit to not have this change

martinvonz: I'll update the queued commit to not have this change
# #
# mercurial - scalable distributed SCM # mercurial - scalable distributed SCM
# #
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import

mercurial/utils/procutil.py

import contextlib import contextlib
import errno import errno
import io import io
import os import os
import signal import signal
import subprocess import subprocess
import sys import sys
import threading
import time import time
from ..i18n import _ from ..i18n import _
from ..pycompat import ( from ..pycompat import (
getattr, getattr,
open, open,
) )
gui() gui()
# double-fork to completely detach from the parent process # double-fork to completely detach from the parent process
# based on http://code.activestate.com/recipes/278731 # based on http://code.activestate.com/recipes/278731
if record_wait is None: if record_wait is None:
pid = os.fork() pid = os.fork()
if pid: if pid:
if not ensurestart: if not ensurestart:
# Even though we're not waiting on the child process,
# we still must call waitpid() on it at some point so
# it's not a zombie/defunct. This is especially relevant for
# chg since the parent process won't die anytime soon.
# We use a thread to make the overhead tiny.
def _do_wait():
os.waitpid(pid, 0)
threading.Thread(target=_do_wait, daemon=True).start()
return return
# Parent process # Parent process
(_pid, status) = os.waitpid(pid, 0) (_pid, status) = os.waitpid(pid, 0)
if os.WIFEXITED(status): if os.WIFEXITED(status):
returncode = os.WEXITSTATUS(status) returncode = os.WEXITSTATUS(status)
else: else:
returncode = -(os.WTERMSIG(status)) returncode = -(os.WTERMSIG(status))
if returncode != 0: if returncode != 0: