Repository: tomMoral/loky

Issue:
_pid attribute of SemaphoreTracker is not passed to child
Since #106, workers created by `loky` inherit their parent's semaphore tracker. However, it seems that the `_pid` attribute gets lost. This test here fails. I'll add a patch soon.

```python
from loky import ProcessPoolExecutor
import loky.backend.semaphore_tracker


def get_semaphore_pid():
    sem_tracker = loky.backend.semaphore_tracker._semaphore_tracker
    sem_tracker.ensure_running()
    return sem_tracker._pid


def test_child_uses_parent_semaphore_tracker():
    parent_semtracker_pid = get_semaphore_pid()
    executor = ProcessPoolExecutor(max_workers=2)
    child_semtracker_pid = executor.submit(get_semaphore_pid).result()
    assert parent_semtracker_pid == child_semtracker_pid


if __name__ == "__main__":
    test_child_uses_parent_semaphore_tracker()
```

Discussion:
pierreglaser: A couple updates:
Something stricked me in `backend.context`:

https://github.com/tomMoral/loky/blob/1f5f77eb18735831a80c9936be52c831d12bcaa5/loky/backend/context.py#L212-L215

This actually imports the `multiprocessing.synchronize` module because of 

https://github.com/tomMoral/loky/blob/1f5f77eb18735831a80c9936be52c831d12bcaa5/loky/backend/__init__.py#L12-L14

in the `__init__.py`

It is the only object in the `context` file for which it is done that waym the rest being done using `from .syncronize import ...`, which imports the local `loky.backend.synchronize` module. Parallely, the following fails:

```python
In [1]: from loky.backend import get_context 
   ...: ctx=get_context('loky') 
   ...: ctx.Semaphore(1)                                                                                                                                                                                    
```
with
```python-traceback
TypeError                                 Traceback (most recent call last)
<ipython-input-1-6669e5978e3a> in <module>
      1 from loky.backend import get_context
      2 ctx=get_context('loky')
----> 3 ctx.Semaphore(1)

~/repos/loky/loky/backend/context.py in Semaphore(self, value)
    213             """Returns a semaphore object"""
    214             from . import synchronize
--> 215             return synchronize.Semaphore(value=value)
    216 
    217         def BoundedSemaphore(self, value):

TypeError: __init__() missing 1 required keyword-only argument: 'ctx'
```

Which makes me think this behavior was not intended although I might be wrong.

I fixed it in https://github.com/tomMoral/loky/pull/202/commits/22e973ae2bc0475610f0f23b2c40e58b2a10a469
pierreglaser: Also, we need to incorporate some new patches of `multiprocessing.semaphore_tracker`. 
I know of [bpo-33613](https://bugs.python.org/issue33613), there may be some others.

Patch:
diff --git a/loky/backend/context.py b/loky/backend/context.py
index 0f744c5..dfeb4ed 100644
--- a/loky/backend/context.py
+++ b/loky/backend/context.py
@@ -211,8 +211,8 @@ class LokyContext(BaseContext):
         """
         def Semaphore(self, value=1):
             """Returns a semaphore object"""
-            from . import synchronize
-            return synchronize.Semaphore(value=value)
+            from .synchronize import Semaphore
+            return Semaphore(value=value)
 
         def BoundedSemaphore(self, value):
             """Returns a bounded semaphore object"""
diff --git a/loky/backend/reduction.py b/loky/backend/reduction.py
index 0d40c5e..0bad5f6 100644
--- a/loky/backend/reduction.py
+++ b/loky/backend/reduction.py
@@ -188,7 +188,11 @@ def set_loky_pickler(loky_pickler=None):
             if sys.version_info < (3,):
                 self.dispatch = self._dispatch.copy()
             else:
-                self.dispatch_table = self._dispatch_table.copy()
+                if getattr(self, "dispatch_table", None) is not None:
+                    self.dispatch_table.update(self._dispatch_table.copy())
+                else:
+                    self.dispatch_table = self._dispatch_table.copy()
+
             for type, reduce_func in reducers.items():
                 self.register(type, reduce_func)
 
diff --git a/loky/backend/semaphore_tracker.py b/loky/backend/semaphore_tracker.py
index 7d3f23e..c83b8c6 100644
--- a/loky/backend/semaphore_tracker.py
+++ b/loky/backend/semaphore_tracker.py
@@ -37,10 +37,13 @@ except ImportError:
     from .semlock import sem_unlink
 
 if sys.version_info < (3,):
-    BrokenPipeError = IOError
+    BrokenPipeError = OSError
 
 __all__ = ['ensure_running', 'register', 'unregister']
 
+_HAVE_SIGMASK = hasattr(signal, 'pthread_sigmask')
+_IGNORED_SIGNALS = (signal.SIGINT, signal.SIGTERM)
+
 VERBOSE = False
 
 
@@ -68,6 +71,13 @@ class SemaphoreTracker(object):
                     return
                 # => dead, launch it again
                 os.close(self._fd)
+                try:
+                    # Clean-up to avoid dangling processes.
+                    os.waitpid(self._pid, 0)
+                except OSError:
+                    # The process was terminated or is a child from an ancestor
+                    # of the current process.
+                    pass
                 self._fd = None
                 self._pid = None
 
@@ -80,8 +90,9 @@ class SemaphoreTracker(object):
             except Exception:
                 pass
 
-            cmd = 'from {} import main; main(%d)'.format(main.__module__)
             r, w = os.pipe()
+            cmd = 'from {} import main; main({}, {})'.format(
+                main.__module__, r, VERBOSE)
             try:
                 fds_to_pass.append(r)
                 # process will out live us, so no need to wait on pid
@@ -94,9 +105,23 @@ class SemaphoreTracker(object):
                     import re
                     for i in range(1, len(args)):
                         args[i] = re.sub("-R+", "-R", args[i])
-                args += ['-c', cmd % r]
+                args += ['-c', cmd]
                 util.debug("launching Semaphore tracker: {}".format(args))
-                pid = spawnv_passfds(exe, args, fds_to_pass)
+                # bpo-33613: Register a signal mask that will block the
+                # signals.  This signal mask will be inherited by the child
+                # that is going to be spawned and will protect the child from a
+                # race condition that can make the child die before it
+                # registers signal handlers for SIGINT and SIGTERM. The mask is
+                # unregistered after spawning the child.
+                try:
+                    if _HAVE_SIGMASK:
+                        signal.pthread_sigmask(signal.SIG_BLOCK,
+                                               _IGNORED_SIGNALS)
+                    pid = spawnv_passfds(exe, args, fds_to_pass)
+                finally:
+                    if _HAVE_SIGMASK:
+                        signal.pthread_sigmask(signal.SIG_UNBLOCK,
+                                               _IGNORED_SIGNALS)
             except BaseException:
                 os.close(w)
                 raise
@@ -142,19 +167,22 @@ unregister = _semaphore_tracker.unregister
 getfd = _semaphore_tracker.getfd
 
 
-def main(fd):
+def main(fd, verbose=0):
     '''Run semaphore tracker.'''
     # protect the process from ^C and "killall python" etc
     signal.signal(signal.SIGINT, signal.SIG_IGN)
     signal.signal(signal.SIGTERM, signal.SIG_IGN)
 
+    if _HAVE_SIGMASK:
+        signal.pthread_sigmask(signal.SIG_UNBLOCK, _IGNORED_SIGNALS)
+
     for f in (sys.stdin, sys.stdout):
         try:
             f.close()
         except Exception:
             pass
 
-    if VERBOSE:  # pragma: no cover
+    if verbose:  # pragma: no cover
         sys.stderr.write("Main semaphore tracker is running\n")
         sys.stderr.flush()
 
@@ -168,14 +196,14 @@ def main(fd):
                     if cmd == b'REGISTER':
                         name = name.decode('ascii')
                         cache.add(name)
-                        if VERBOSE:  # pragma: no cover
+                        if verbose:  # pragma: no cover
                             sys.stderr.write("[SemaphoreTracker] register {}\n"
                                              .format(name))
                             sys.stderr.flush()
                     elif cmd == b'UNREGISTER':
                         name = name.decode('ascii')
                         cache.remove(name)
-                        if VERBOSE:  # pragma: no cover
+                        if verbose:  # pragma: no cover
                             sys.stderr.write("[SemaphoreTracker] unregister {}"
                                              ": cache({})\n"
                                              .format(name, len(cache)))
@@ -205,16 +233,16 @@ def main(fd):
             try:
                 try:
                     sem_unlink(name)
-                    if VERBOSE:  # pragma: no cover
+                    if verbose:  # pragma: no cover
                         sys.stderr.write("[SemaphoreTracker] unlink {}\n"
                                          .format(name))
                         sys.stderr.flush()
                 except Exception as e:
-                    warnings.warn('semaphore_tracker: %r: %r' % (name, e))
+                    warnings.warn('semaphore_tracker: %s: %r' % (name, e))
             finally:
                 pass
 
-    if VERBOSE:  # pragma: no cover
+    if verbose:  # pragma: no cover
         sys.stderr.write("semaphore tracker shut down\n")
         sys.stderr.flush()
 
diff --git a/loky/backend/spawn.py b/loky/backend/spawn.py
index fb375e5..c0390e4 100644
--- a/loky/backend/spawn.py
+++ b/loky/backend/spawn.py
@@ -151,7 +151,7 @@ def prepare(data):
     if 'orig_dir' in data:
         process.ORIGINAL_DIR = data['orig_dir']
 
-    if 'tacker_pid' in data:
+    if 'tracker_pid' in data:
         from . import semaphore_tracker
         semaphore_tracker._semaphore_tracker._pid = data["tracker_pid"]
 


Test patch:
diff --git a/tests/test_semaphore_tracker.py b/tests/test_semaphore_tracker.py
new file mode 100644
index 0000000..9e04a1d
--- /dev/null
+++ b/tests/test_semaphore_tracker.py
@@ -0,0 +1,199 @@
+"""Tests for the SemaphoreTracker class"""
+import errno
+import gc
+import io
+import os
+import pytest
+import re
+import signal
+import sys
+import time
+import warnings
+import weakref
+
+from loky import ProcessPoolExecutor
+import loky.backend.semaphore_tracker as semaphore_tracker
+from loky.backend.semlock import sem_unlink
+from loky.backend.context import get_context
+
+
+def get_sem_tracker_pid():
+    semaphore_tracker.ensure_running()
+    return semaphore_tracker._semaphore_tracker._pid
+
+
+@pytest.mark.skipif(sys.platform == "win32",
+                    reason="no semaphore_tracker on windows")
+class TestSemaphoreTracker:
+    def test_child_retrieves_semaphore_tracker(self):
+        parent_sem_tracker_pid = get_sem_tracker_pid()
+        executor = ProcessPoolExecutor(max_workers=1)
+
+        # Register a semaphore in the parent process, and un-register it in the
+        # child process. If the two processes do not share the same
+        # semaphore_tracker, a cache KeyError should be printed in stderr.
+        import subprocess
+        semlock_name = 'loky-mysemaphore'
+        cmd = '''if 1:
+        import os, sys
+
+        from loky import ProcessPoolExecutor
+        from loky.backend import semaphore_tracker
+        from loky.backend.semlock import SemLock
+
+        semaphore_tracker.VERBOSE=True
+        semlock_name = "{}"
+
+        # We don't need to create the semaphore as registering / unregistering
+        # operations simply add / remove entries from a cache, but do not
+        # manipulate the actual semaphores.
+        semaphore_tracker.register(semlock_name)
+
+        def unregister(name):
+            # semaphore_tracker.unregister is actually a bound method of the
+            # SemaphoreTracker. We need a custom wrapper to avoid object
+            # serialization.
+            from loky.backend import semaphore_tracker
+            semaphore_tracker.unregister(semlock_name)
+
+        e = ProcessPoolExecutor(1)
+        e.submit(unregister, semlock_name).result()
+        e.shutdown()
+        '''
+        try:
+            child_sem_tracker_pid = executor.submit(
+                get_sem_tracker_pid).result()
+
+            # First simple pid retrieval check (see #200)
+            assert child_sem_tracker_pid == parent_sem_tracker_pid
+
+            p = subprocess.Popen(
+                [sys.executable, '-E', '-c', cmd.format(semlock_name)],
+                stderr=subprocess.PIPE)
+            p.wait()
+
+            err = p.stderr.read().decode('utf-8')
+            p.stderr.close()
+
+            assert re.search("unregister %s" % semlock_name, err) is not None
+            assert re.search("KeyError: '%s'" % semlock_name, err) is None
+
+        finally:
+            executor.shutdown()
+
+
+    # The following four tests are inspired from cpython _test_multiprocessing
+    def test_semaphore_tracker(self):
+        #
+        # Check that killing process does not leak named semaphores
+        #
+        import subprocess
+        cmd = '''if 1:
+            import time, os
+            from loky.backend.synchronize import Lock
+
+            # close manually the read end of the pipe in the child process
+            # because pass_fds does not exist for python < 3.2
+            os.close(%d)
+
+            lock1 = Lock()
+            lock2 = Lock()
+            os.write(%d, lock1._semlock.name.encode("ascii") + b"\\n")
+            os.write(%d, lock2._semlock.name.encode("ascii") + b"\\n")
+            time.sleep(10)
+        '''
+        r, w = os.pipe()
+
+        if sys.version_info[:2] >= (3, 2):
+            fd_kws = {'pass_fds': [w, r]}
+        else:
+            fd_kws = {'close_fds': False}
+        p = subprocess.Popen([sys.executable,
+                             '-E', '-c', cmd % (r, w, w)],
+                             stderr=subprocess.PIPE,
+                             **fd_kws)
+        os.close(w)
+        with io.open(r, 'rb', closefd=True) as f:
+            name1 = f.readline().rstrip().decode('ascii')
+            name2 = f.readline().rstrip().decode('ascii')
+
+        # subprocess holding a reference to lock1 is still alive, so this call
+        # should succeed
+        sem_unlink(name1)
+        p.terminate()
+        p.wait()
+        time.sleep(2.0)
+        with pytest.raises(OSError) as ctx:
+            sem_unlink(name2)
+        # docs say it should be ENOENT, but OSX seems to give EINVAL
+        assert ctx.value.errno in (errno.ENOENT, errno.EINVAL)
+        err = p.stderr.read().decode('utf-8')
+        p.stderr.close()
+        expected = ('semaphore_tracker: There appear to be 2 leaked '
+                    'semaphores')
+        assert re.search(expected, err) is not None
+
+        # lock1 is still registered, but was destroyed externally: the tracker
+        # is expected to complain.
+        expected = ("semaphore_tracker: %s: (OSError\\(%d|"
+                    "FileNotFoundError)" % (name1, errno.ENOENT))
+        assert re.search(expected, err) is not None
+
+    def check_semaphore_tracker_death(self, signum, should_die):
+        # bpo-31310: if the semaphore tracker process has died, it should
+        # be restarted implicitly.
+        from loky.backend.semaphore_tracker import _semaphore_tracker
+        pid = _semaphore_tracker._pid
+        if pid is not None:
+            os.kill(pid, signal.SIGKILL)
+            os.waitpid(pid, 0)
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore")
+            _semaphore_tracker.ensure_running()
+            # in python < 3.3 , the race condition described in bpo-33613 still
+            # exists, as this fixe requires signal.pthread_sigmask
+            time.sleep(1.0)
+        pid = _semaphore_tracker._pid
+
+        os.kill(pid, signum)
+        time.sleep(1.0)  # give it time to die
+
+        ctx = get_context("loky")
+        with warnings.catch_warnings(record=True) as all_warn:
+            warnings.simplefilter("always")
+
+            # remove unrelated MacOS warning messages first
+            warnings.filterwarnings(
+                "ignore", message='semaphore are broken on OSX')
+
+            sem = ctx.Semaphore()
+            sem.acquire()
+            sem.release()
+            wr = weakref.ref(sem)
+            # ensure `sem` gets collected, which triggers communication with
+            # the semaphore tracker
+            del sem
+            gc.collect()
+            assert wr() is None
+            if should_die:
+                assert len(all_warn) == 1
+                the_warn = all_warn[0]
+                assert issubclass(the_warn.category, UserWarning)
+                assert "semaphore_tracker: process died" in str(
+                    the_warn.message)
+            else:
+                assert len(all_warn) == 0
+
+    def test_semaphore_tracker_sigint(self):
+        # Catchable signal (ignored by semaphore tracker)
+        self.check_semaphore_tracker_death(signal.SIGINT, False)
+
+    def test_semaphore_tracker_sigterm(self):
+        # Catchable signal (ignored by semaphore tracker)
+        self.check_semaphore_tracker_death(signal.SIGTERM, False)
+
+    @pytest.mark.skipif(sys.version_info[0] < 3,
+                        reason="warnings.catch_warnings limitation")
+    def test_semaphore_tracker_sigkill(self):
+        # Uncatchable signal.
+        self.check_semaphore_tracker_death(signal.SIGKILL, True)