Merge pull request #265 from pytest-dev/remove-mp-finalizer

Remove multiprocess finalizer and improve subprocess docs

appveyor.yml

@@ -8,7 +8,7 @@ environment:
     - TOXENV: 'py27-t310-c45,py27-t40-c45,py27-t41-c45'
     - TOXENV: 'py34-t310-c45,py34-t40-c45,py34-t41-c45'
     - TOXENV: 'py35-t310-c45,py35-t40-c45,py35-t41-c45'
-    - TOXENV: 'pypy-t310-c45,pypy-t40-c45,pypy-t41-c45,pypy3-t310-c45,pypy3-t40-c45,pypy3-t41-c45'
+    - TOXENV: 'pypy-t310-c45,pypy-t40-c45,pypy-t41-c45'
 
 init:
   - ps: echo $env:TOXENV

ci/bootstrap.py

@@ -52,7 +52,7 @@
 
     template_vars = {'tox_environments': tox_environments}
     for py_ver in '27 34 35 py'.split():
-        template_vars['py%s_environments' % py_ver] = [x for x in tox_environments if x.startswith('py' + py_ver)]
+        template_vars['py%s_environments' % py_ver] = [x for x in tox_environments if x.startswith('py' + py_ver + '-')]
 
     for name in os.listdir(join("ci", "templates")):
         with open(join(base_path, name), "w") as fh:

docs/index.rst

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

docs/plugins.rst

@@ -14,9 +14,9 @@ Alternatively you can have this in ``tox.ini`` (if you're using `Tox <https://to
 
     [testenv]
     setenv =
-        COV_CORE_SOURCE={toxinidir}/src
+        COV_CORE_SOURCE=
         COV_CORE_CONFIG={toxinidir}/.coveragerc
-        COV_CORE_DATAFILE={toxinidir}/.coverage.eager
+        COV_CORE_DATAFILE={toxinidir}/.coverage
 
 And in ``pytest.ini`` / ``tox.ini`` / ``setup.cfg``::
 

docs/subprocess-support.rst

@@ -0,0 +1,169 @@
+==================
+Subprocess support
+==================
+
+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.
+
+pytest-cov supports subprocesses and multiprocessing, and works around these atexit limitations. However, there are a
+few pitfalls that need to be explained.
+
+If you use ``multiprocessing.Pool``
+===================================
+
+**pytest-cov** automatically registers a multiprocessing finalizer. The finalizer will only run reliably if the pool is
+closed. Closing the pool basically signals the workers that there will be no more work, and they will eventually exit.
+Thus one also needs to call `join` on the pool.
+
+If you use ``multiprocessing.Pool.terminate`` or the context manager API (``__exit__``
+will just call ``terminate``) then the workers can get SIGTERM and then the finalizers won't run or complete in time.
+Thus 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:
+            p.close()  # Marks the pool as closed.
+            p.join()   # Waits for workers to exit.
+
+
+If you must use the context manager API (e.g.: the pool is managed in third party code you can't change) then you can
+register a cleaning SIGTERM handler like so:
+
+.. code-block:: python
+
+    from multiprocessing import Pool
+
+    def f(x):
+        return x*x
+
+    if __name__ == '__main__':
+        try:
+            from pytest_cov.embed import cleanup_on_sigterm
+        except ImportError:
+            pass
+        else:
+            cleanup_on_sigterm()
+
+        with Pool(5) as p:
+            print(p.map(f, [1, 2, 3]))
+
+If you use ``multiprocessing.Process``
+======================================
+
+There's similar 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__':
+        try:
+            from pytest_cov.embed import cleanup_on_sigterm
+        except ImportError:
+            pass
+        else:
+            cleanup_on_sigterm()
+
+        p = Process(target=f, args=('bob',))
+        try:
+            p.start()
+        finally:
+            p.join()  # necessary so that the Process exists before the test suite exits (thus coverage is collected)
+
+.. _cleanup_on_sigterm:
+
+If you got custom signal handling
+=================================
+
+**pytest-cov 2.6** has a rudimentary ``pytest_cov.embed.cleanup_on_sigterm`` you can use to register a SIGTERM handler
+that flushes the coverage data.
+
+**pytest-cov 2.7** adds a ``pytest_cov.embed.cleanup_on_signal`` function and changes the implementation to be more
+robust: the handler will call the previous handler (if you had previously registered any), and is re-entrant (will
+defer extra signals if delivered while the handler runs).
+
+For example, if you 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:
+
+.. code-block:: python
+
+    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)
+
+If you use Windows
+==================
+
+On Windows you can register a handler for SIGTERM but it doesn't actually work. It will work if you
+`os.kill(os.getpid(), signal.SIGTERM)` (send SIGTERM to the current process) but for most intents and purposes that's
+completely useless.
+
+Consequently this means that if you use multiprocessing you got no choice but to use the close/join pattern as described
+above. Using the context manager API or `terminate` won't work as it relies on SIGTERM.
+
+However you can have a working handler for SIGBREAK (with some caveats):
+
+.. code-block:: python
+
+    import os
+    import signal
+
+    def shutdown(frame, signum):
+        # your app's shutdown or whatever
+    signal.signal(signal.SIGBREAK, shutdown)
+
+    try:
+        from pytest_cov.embed import cleanup_on_signal
+    except ImportError:
+        pass
+    else:
+        cleanup_on_signal(signal.SIGBREAK)
+
+The `caveats <https://stefan.sofa-rockers.org/2013/08/15/handling-sub-process-hierarchies-python-linux-os-x/>`_ being
+roughly:
+
+* you need to deliver ``signal.CTRL_BREAK_EVENT``
+* it gets delivered to the whole process group, and that can have unforeseen consequences

src/pytest_cov/embed.py

@@ -13,16 +13,19 @@
 that code coverage is being collected we activate coverage based on
 info passed via env vars.
 """
+import atexit
 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)
 
 
 try:
@@ -36,27 +39,29 @@ 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')
     cov_datafile = os.environ.get('COV_CORE_DATAFILE')
     cov_branch = True if os.environ.get('COV_CORE_BRANCH') == 'enabled' else None
 
     if cov_datafile:
+        if _active_cov:
+            cleanup()
         # Import what we need to activate coverage.
         import coverage
 
         # Determine all source roots.
-        if cov_source == os.pathsep:
+        if cov_source in os.pathsep:
             cov_source = None
         else:
             cov_source = cov_source.split(os.pathsep)
         if cov_config == os.pathsep:
             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,
@@ -75,28 +80,44 @@ def _cleanup(cov):
     if cov is not None:
         cov.stop()
         cov.save()
+        cov._auto_save = False  # prevent autosaving from cov._atexit in case the interpreter lacks atexit.unregister
+        try:
+            atexit.unregister(cov._atexit)
+        except Exception:
+            pass
 
 
-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
+    _cleanup_in_progress = False
+    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 +126,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():