From patchwork Thu May  7 10:18:50 2020
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: D8497: procutil: always waiting on child processes to prevent zombies
 with 'hg serve'
From: phabricator <phabricator@mercurial-scm.org>
X-Patchwork-Id: 46263
Message-Id: <differential-rev-PHID-DREV-ilfg52ja46piw4e4rnvn-req@mercurial-scm.org>
To: Phabricator <phabricator@mercurial-scm.org>
Cc: mercurial-devel@mercurial-scm.org
Date: Thu, 7 May 2020 10:18:50 +0000

rdamazio created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION 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.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8497

AFFECTED FILES
  hg
  mercurial/utils/procutil.py

CHANGE DETAILS




To: rdamazio, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

diff --git a/mercurial/utils/procutil.py b/mercurial/utils/procutil.py
--- a/mercurial/utils/procutil.py
+++ b/mercurial/utils/procutil.py
@@ -16,6 +16,7 @@
 import signal
 import subprocess
 import sys
+import threading
 import time
 
 from ..i18n import _
@@ -604,6 +605,14 @@
             pid = os.fork()
             if pid:
                 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
                 # Parent process
                 (_pid, status) = os.waitpid(pid, 0)
diff --git a/hg b/hg
--- a/hg
+++ b/hg
@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 #
 # mercurial - scalable distributed SCM
 #