semaphore tracker isn't protected against crashes

[pitrou]

BPO	31310
Nosy	@pitrou, @serhiy-storchaka, @applio, @tomMoral, @pablogsal
PRs	bpo-31310: multiprocessing's semaphore tracker should be launched again if crashed #3247 [3.6] bpo-31310: multiprocessing's semaphore tracker should be launched again if crashed (GH-3247) #4254 bpo-36668: FIX reuse semaphore tracker for child processes #5172

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-10-06.13:15:03.035>
created_at = <Date 2017-08-30.15:52:32.836>
labels = ['3.7', 'type-bug', 'library']
title = "semaphore tracker isn't protected against crashes"
updated_at = <Date 2018-10-06.13:15:03.034>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2018-10-06.13:15:03.034>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2018-10-06.13:15:03.035>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2017-08-30.15:52:32.836>
creator = 'pitrou'
dependencies = []
files = []
hgrepos = []
issue_num = 31310
keywords = ['patch']
message_count = 10.0
messages = ['301029', '305480', '305485', '309888', '309889', '309890', '309893', '309895', '326827', '327243']
nosy_count = 5.0
nosy_names = ['pitrou', 'serhiy.storchaka', 'davin', 'tomMoral', 'pablogsal']
pr_nums = ['3247', '4254', '5172']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue31310'
versions = ['Python 3.6', 'Python 3.7']

[pitrou]

Similar to bpo-31308, but pertaining to multiprocessing's semaphore tracker process.

[pitrou]

New changeset cbe1756 by Antoine Pitrou in branch 'master':
bpo-31310: multiprocessing's semaphore tracker should be launched again if crashed (bpo-3247)
cbe1756

[pitrou]

New changeset b5f09ac by Antoine Pitrou in branch '3.6':
[3.6] bpo-31310: multiprocessing's semaphore tracker should be launched again if crashed (GH-3247) (bpo-4254)
b5f09ac

[tomMoral]

With this fix, the semaphore_tracker is not shared with the children anymore and each process launches its own tracker.

I opened a PR to try to fix it. Let me know if I should open a new ticket.

[pitrou]

With this fix, the semaphore_tracker is not shared with the children anymore and each process launches its own tracker.

That's only if the semaphore tracker process crashes, right?

[tomMoral]

For new processes created with spawn or forkserver, only the semaphore_tracker._fd is send and shared to the child. Thus, as the _pid argument is None in the new process, it launches a new tracker if it needs to create a new Semaphore, regardless of crashes in the previous semaphore tracker.

[pitrou]

Thus, as the _pid argument is None in the new process, it launches a new tracker if it needs to create a new Semaphore

I see. Is it an optimization problem, or does it risk leaking semaphores? The problem is if a semaphore shared with the child isn't garbage collected correctly.

[tomMoral]

Is it an optimization problem, or does it risk leaking semaphores?

I do not think it risks leaking semaphores as the clean-up is performed by the process which created the Semaphore. So I would say it is more an optimization issue.

It is true that I do not see many use case where the Semaphore might be created by the child process but it was optimized in previous version. If you don't think it is useful, we could avoid sharing the semaphore_tracker pipe to the child process to reduce complexity in the spawning process. Do you think it is worth fixing or it should be simplified?

[serhiy-storchaka]

Tests are failed when ran with -Werror.

$ ./python -We -m test -vuall test_multiprocessing_fork test_multiprocessing_forkserver test_multiprocessing_spawn
...

======================================================================
ERROR: test_semaphore_tracker_sigkill (test.test_multiprocessing_fork.TestSemaphoreTracker)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4582, in test_semaphore_tracker_sigkill
    self.check_semaphore_tracker_death(signal.SIGKILL, True)
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4546, in check_semaphore_tracker_death
    _semaphore_tracker.ensure_running()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 64, in ensure_running
    warnings.warn('semaphore_tracker: process died unexpectedly, '
UserWarning: semaphore_tracker: process died unexpectedly, relaunching.  Some semaphores might leak.

---

...
======================================================================
ERROR: test_semaphore_tracker_sigint (test.test_multiprocessing_forkserver.TestSemaphoreTracker)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4574, in test_semaphore_tracker_sigint
    self.check_semaphore_tracker_death(signal.SIGINT, False)
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4546, in check_semaphore_tracker_death
    _semaphore_tracker.ensure_running()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 64, in ensure_running
    warnings.warn('semaphore_tracker: process died unexpectedly, '
UserWarning: semaphore_tracker: process died unexpectedly, relaunching.  Some semaphores might leak.

======================================================================
ERROR: test_semaphore_tracker_sigkill (test.test_multiprocessing_forkserver.TestSemaphoreTracker)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4582, in test_semaphore_tracker_sigkill
    self.check_semaphore_tracker_death(signal.SIGKILL, True)
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4554, in check_semaphore_tracker_death
    sem = ctx.Semaphore()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/context.py", line 82, in Semaphore
    return Semaphore(value, ctx=self.get_context())
  File "/home/serhiy/py/cpython/Lib/multiprocessing/synchronize.py", line 126, in __init__
    SemLock.__init__(self, SEMAPHORE, value, SEM_VALUE_MAX, ctx=ctx)
  File "/home/serhiy/py/cpython/Lib/multiprocessing/synchronize.py", line 80, in __init__
    register(self._semlock.name)
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 104, in register
    self._send('REGISTER', name)
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 111, in _send
    self.ensure_running()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 64, in ensure_running
    warnings.warn('semaphore_tracker: process died unexpectedly, '
UserWarning: semaphore_tracker: process died unexpectedly, relaunching.  Some semaphores might leak.

---

...
======================================================================
ERROR: test_semaphore_tracker_sigint (test.test_multiprocessing_spawn.TestSemaphoreTracker)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4574, in test_semaphore_tracker_sigint
    self.check_semaphore_tracker_death(signal.SIGINT, False)
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4546, in check_semaphore_tracker_death
    _semaphore_tracker.ensure_running()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 64, in ensure_running
    warnings.warn('semaphore_tracker: process died unexpectedly, '
UserWarning: semaphore_tracker: process died unexpectedly, relaunching.  Some semaphores might leak.

======================================================================
ERROR: test_semaphore_tracker_sigkill (test.test_multiprocessing_spawn.TestSemaphoreTracker)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4582, in test_semaphore_tracker_sigkill
    self.check_semaphore_tracker_death(signal.SIGKILL, True)
  File "/home/serhiy/py/cpython/Lib/test/_test_multiprocessing.py", line 4554, in check_semaphore_tracker_death
    sem = ctx.Semaphore()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/context.py", line 82, in Semaphore
    return Semaphore(value, ctx=self.get_context())
  File "/home/serhiy/py/cpython/Lib/multiprocessing/synchronize.py", line 126, in __init__
    SemLock.__init__(self, SEMAPHORE, value, SEM_VALUE_MAX, ctx=ctx)
  File "/home/serhiy/py/cpython/Lib/multiprocessing/synchronize.py", line 80, in __init__
    register(self._semlock.name)
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 104, in register
    self._send('REGISTER', name)
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 111, in _send
    self.ensure_running()
  File "/home/serhiy/py/cpython/Lib/multiprocessing/semaphore_tracker.py", line 64, in ensure_running
    warnings.warn('semaphore_tracker: process died unexpectedly, '
UserWarning: semaphore_tracker: process died unexpectedly, relaunching.  Some semaphores might leak.

---

[serhiy-storchaka]

Sorry, that bug was actually introduced in bpo-33613.