Remove multiprocess finalizer and improve subprocess docs

docs/index.rst

@@ -11,7 +11,7 @@ Contents:
     reporting
     debuggers
     xdist
-    mp
+    subprocess-support
     plugins
     markers-fixtures
     changelog

docs/subprocess-support.rst

@@ -0,0 +1,114 @@
+==================
+Subprocess support
+==================
+
+pytest-cov supports subprocesses and multiprocessing. However, there are a few pitfalls that need to be
+explained.
+
+Normally coverage writes the data via a pretty standard atexit handler. However, if the subprocess doesn't exit on its
+own then the atexit handler might not run. Why that happens is best left to the adventurous to discover by waddling
+though the Python bug tracker.
+
+For now pytest-cov provides opt-in workarounds for these problems.
+
+If you use ``multiprocessing.Pool``
+===================================
+
+You need to make sure your ``multiprocessing.Pool`` gets a nice and clean exit:
+
+.. code-block:: python
+
+    from multiprocessing import Pool
+
+    def f(x):
+        return x*x
+
+    if __name__ == '__main__':
+        p = Pool(5)
+        try:
+            print(p.map(f, [1, 2, 3]))
+        finally:       # <= THIS IS ESSENTIAL
+            p.close()  # <= THIS IS ESSENTIAL
+            p.join()   # <= THIS IS ESSENTIAL
+
+Previously this guide recommended using ``multiprocessing.Pool``'s context manager API, however, that was wrong as
+``multiprocessing.Pool.__exit__`` is an alias to ``multiprocessing.Pool.terminate``, and that doesn't always run the
+finalizers (sometimes the problem in `cleanup_on_sigterm`_ will appear).
+
+If you use ``multiprocessing.Process``
+======================================
+
+There's an identical issue when using the ``Process`` objects. Don't forget to use ``.join()``:
+
+.. code-block:: python
+
+    from multiprocessing import Process
+
+    def f(name):
+        print('hello', name)
+
+    if __name__ == '__main__':
+        p = Process(target=f, args=('bob',))
+        try:
+            p.start()
+        finally:      # <= THIS IS ESSENTIAL
+            p.join()  # <= THIS IS ESSENTIAL
+
+.. _cleanup_on_sigterm:
+
+If you abuse ``multiprocessing.Process.terminate``
+==================================================
+
+It appears that many people are using the ``terminate`` method and then get unreliable coverage results. That usually
+means a SIGTERM gets sent to the process. Unfortunately Python don't have a default handler for SIGTERM so you need to
+install your own. Because ``pytest-cov`` doesn't want to second-guess (not yet, add your thoughts on the issue tracker
+if you disagree) it doesn't install a handler by default, but you can activate it by doing this:
+
+.. code-block:: python
+
+    try:
+        from pytest_cov.embed import cleanup_on_sigterm
+    except ImportError:
+        pass
+    else:
+        cleanup_on_sigterm()
+
+If anything else
+================
+
+If you have custom signal handling, eg: you do reload on SIGHUP you should have something like this:
+
+.. code-block:: python
+
+    import os
+    import signal
+
+    def restart_service(frame, signum):
+        os.exec( ... )  # or whatever your custom signal would do
+    signal.signal(signal.SIGHUP, restart_service)
+
+    try:
+        from pytest_cov.embed import cleanup_on_signal
+    except ImportError:
+        pass
+    else:
+        cleanup_on_signal(signal.SIGHUP)
+
+Note that both ``cleanup_on_signal`` and ``cleanup_on_sigterm`` will run the previous signal handler.
+
+Alternatively you can do this:
+
+    import os
+    import signal
+
+    try:
+        from pytest_cov.embed import cleanup
+    except ImportError:
+        cleanup = None
+
+    def restart_service(frame, signum):
+        if cleanup is not None:
+            cleanup()
+
+        os.exec( ... )  # or whatever your custom signal would do
+    signal.signal(signal.SIGHUP, restart_service)

src/pytest_cov/embed.py

@@ -16,13 +16,16 @@
 import os
 import signal
 
-active_cov = None
+_active_cov = None
 
 
 def multiprocessing_start(_):
+    global _active_cov
     cov = init()
     if cov:
-        multiprocessing.util.Finalize(None, cleanup, args=(cov,), exitpriority=1000)
+        _active_cov = cov
+        multiprocessing.util.Finalize(None, cleanup, exitpriority=1000)
+        cleanup_on_sigterm()
 
 
 try:
@@ -36,7 +39,7 @@ def multiprocessing_start(_):
 def init():
     # Only continue if ancestor process has set everything needed in
     # the env.
-    global active_cov
+    global _active_cov
 
     cov_source = os.environ.get('COV_CORE_SOURCE')
     cov_config = os.environ.get('COV_CORE_CONFIG')
@@ -56,7 +59,7 @@ def init():
             cov_config = True
 
         # Activate coverage for this process.
-        cov = active_cov = coverage.Coverage(
+        cov = _active_cov = coverage.Coverage(
             source=cov_source,
             branch=cov_branch,
             data_suffix=True,
@@ -77,26 +80,36 @@ def _cleanup(cov):
         cov.save()
 
 
-def cleanup(cov=None):
-    global active_cov
+def cleanup():
+    global _active_cov
+    global _cleanup_in_progress
+    global _pending_signal
 
-    _cleanup(cov)
-    if active_cov is not cov:
-        _cleanup(active_cov)
-    active_cov = None
+    _cleanup_in_progress = True
+    _cleanup(_active_cov)
+    _active_cov = None
+    if _pending_signal:
+        _signal_cleanup_handler(*_pending_signal)
+        _pending_signal = None
 
 
 multiprocessing_finish = cleanup  # in case someone dared to use this internal
 
 _previous_handlers = {}
+_pending_signal = None
+_cleanup_in_progress = False
 
 
 def _signal_cleanup_handler(signum, frame):
+    global _pending_signal
+    if _cleanup_in_progress:
+        _pending_signal = signum, frame
+        return
     cleanup()
     _previous_handler = _previous_handlers.get(signum)
     if _previous_handler == signal.SIG_IGN:
         return
-    elif _previous_handler:
+    elif _previous_handler and _previous_handler is not _signal_cleanup_handler:
         _previous_handler(signum, frame)
     elif signum == signal.SIGTERM:
         os._exit(128 + signum)
@@ -105,8 +118,10 @@ def _signal_cleanup_handler(signum, frame):
 
 
 def cleanup_on_signal(signum):
-    _previous_handlers[signum] = signal.getsignal(signum)
-    signal.signal(signum, _signal_cleanup_handler)
+    previous = signal.getsignal(signum)
+    if previous is not _signal_cleanup_handler:
+        _previous_handlers[signum] = previous
+        signal.signal(signum, _signal_cleanup_handler)
 
 
 def cleanup_on_sigterm():

src/pytest_cov/plugin.py

@@ -118,7 +118,6 @@ def __init__(self, options, pluginmanager, start=True):
 
         # Our implementation is unknown at this time.
         self.pid = None
-        self.cov = None
         self.cov_controller = None
         self.cov_report = compat.StringIO()
         self.cov_total = None
@@ -286,12 +285,10 @@ def pytest_runtest_setup(self, item):
         if os.getpid() != self.pid:
             # test is run in another process than session, run
             # coverage manually
-            self.cov = embed.init()
+            embed.init()
 
     def pytest_runtest_teardown(self, item):
-        if self.cov is not None:
-            embed.cleanup(self.cov)
-            self.cov = None
+        embed.cleanup()
 
     @compat.hookwrapper
     def pytest_runtest_call(self, item):

tests/test_pytest_cov.py

@@ -889,6 +889,38 @@ def test_funcarg_not_active(testdir):
     assert result.ret == 0
 
 
+def test_multiprocessing_pool(testdir):
+    py.test.importorskip('multiprocessing.util')
+
+    script = testdir.makepyfile('''
+import multiprocessing
+
+def target_fn(a):
+    return a + 1
+
+def test_run_target():
+    for i in range(100):
+        with multiprocessing.Pool(10) as p:
+            p.map(target_fn, range(10))
+        p.join()
+''')
+
+    result = testdir.runpytest('-v',
+                               '--cov=%s' % script.dirpath(),
+                               '--cov-report=term-missing',
+                               script)
+
+    result.stdout.fnmatch_lines([
+        '*- coverage: platform *, python * -*',
+        'test_multiprocessing_pool* 8 * 100%*',
+        '*1 passed*'
+    ])
+    assert result.ret == 0
+    # assert "Doesn't seem to be a coverage.py data file" not in result.stdout.str()
+    # assert "Doesn't seem to be a coverage.py data file" not in result.stderr.str()
+    assert not testdir.tmpdir.listdir(".coverage.*")
+
+
 def test_multiprocessing_subprocess(testdir):
     py.test.importorskip('multiprocessing.util')
 
@@ -1112,6 +1144,7 @@ def test_run():
     ])
     assert result.ret == 0
 
+
 @pytest.mark.skipif('sys.platform == "win32"', reason="fork not available on Windows")
 def test_cleanup_on_sigterm_sig_ign(testdir):
     script = testdir.makepyfile('''