classification
Title: semaphore tracker isn't protected against crashes
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: davin, pablogsal, pitrou, serhiy.storchaka, tomMoral
Priority: normal Keywords: patch

Created on 2017-08-30 15:52 by pitrou, last changed 2018-10-06 13:15 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3247 merged pitrou, 2017-08-30 15:56
PR 4254 merged pitrou, 2017-11-03 13:32
PR 5172 open tomMoral, 2018-01-13 10:33
Messages (10)
msg301029 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-08-30 15:52
Similar to bpo-31308, but pertaining to multiprocessing's semaphore tracker process.
msg305480 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-03 13:31
New changeset cbe1756e3ecefc0e24a5d0a4b8663db9b6d0cc52 by Antoine Pitrou in branch 'master':
bpo-31310: multiprocessing's semaphore tracker should be launched again if crashed (#3247)
https://github.com/python/cpython/commit/cbe1756e3ecefc0e24a5d0a4b8663db9b6d0cc52
msg305485 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-11-03 13:58
New changeset b5f09acf0a0219cec32b7eba3acdcb573fc74ab5 by Antoine Pitrou in branch '3.6':
[3.6] bpo-31310: multiprocessing's semaphore tracker should be launched again if crashed (GH-3247) (#4254)
https://github.com/python/cpython/commit/b5f09acf0a0219cec32b7eba3acdcb573fc74ab5
msg309888 - (view) Author: Thomas Moreau (tomMoral) * Date: 2018-01-13 10:39
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.
msg309889 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-01-13 15:36
> 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?
msg309890 - (view) Author: Thomas Moreau (tomMoral) * Date: 2018-01-13 15:51
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.
msg309893 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-01-13 17:41
> 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.
msg309895 - (view) Author: Thomas Moreau (tomMoral) * Date: 2018-01-13 18:06
> 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?
msg326827 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-01 20:43
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.

----------------------------------------------------------------------
msg327243 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-06 13:15
Sorry, that bug was actually introduced in issue33613.
History
Date User Action Args
2018-10-06 13:15:03serhiy.storchakasetstatus: open -> closed

messages: + msg327243
stage: needs patch -> resolved
2018-10-03 18:02:56pablogsalsetnosy: + pablogsal
2018-10-01 20:43:10serhiy.storchakasetstatus: closed -> open

nosy: + serhiy.storchaka
messages: + msg326827

stage: resolved -> needs patch
2018-01-13 18:06:23tomMoralsetmessages: + msg309895
2018-01-13 17:41:34pitrousetmessages: + msg309893
2018-01-13 15:51:15tomMoralsetmessages: + msg309890
2018-01-13 15:36:16pitrousetmessages: + msg309889
2018-01-13 10:39:41tomMoralsetnosy: + tomMoral
messages: + msg309888
2018-01-13 10:34:00tomMoralsetpull_requests: + pull_request5025
2017-11-03 13:58:55pitrousetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-11-03 13:58:40pitrousetmessages: + msg305485
2017-11-03 13:32:36pitrousetkeywords: + patch
pull_requests: + pull_request4216
2017-11-03 13:31:40pitrousetmessages: + msg305480
2017-08-30 19:17:28pitrousetstage: needs patch -> patch review
2017-08-30 15:56:16pitrousetpull_requests: + pull_request3292
2017-08-30 15:52:32pitroucreate