test_multiprocessing_fork: test_semaphore_tracker_sigint() fails with -W error

[vstinner]

BPO	33613
Nosy	@pitrou, @vstinner, @taleinat, @serhiy-storchaka, @applio, @pablogsal
PRs	bpo-33613, test_semaphore_tracker_sigint: fix race condition #7850 bpo-33613: multiprocessing semtracker uses sigmask #8500 [3.7] bpo-33613, test_semaphore_tracker_sigint: fix race condition (GH-7850) #9055 bpo-33613: Fix test_semaphore_tracker signal tests when using -Werror #9778

^{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 = 'https://github.com/pablogsal'
closed_at = <Date 2018-10-10.08:18:02.308>
created_at = <Date 2018-05-23.10:04:09.411>
labels = ['3.8', 'tests']
title = 'test_multiprocessing_fork: test_semaphore_tracker_sigint() fails with -W error'
updated_at = <Date 2018-10-10.08:18:02.307>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2018-10-10.08:18:02.307>
actor = 'pablogsal'
assignee = 'pablogsal'
closed = True
closed_date = <Date 2018-10-10.08:18:02.308>
closer = 'pablogsal'
components = ['Tests']
creation = <Date 2018-05-23.10:04:09.411>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 33613
keywords = ['patch']
message_count = 31.0
messages = ['317372', '317374', '317378', '317380', '320062', '320063', '320113', '320364', '320370', '320371', '320372', '320373', '320374', '320376', '320379', '320381', '320382', '320384', '320614', '324564', '324565', '324567', '324568', '324569', '324570', '324571', '324572', '324573', '324574', '327244', '327459']
nosy_count = 6.0
nosy_names = ['pitrou', 'vstinner', 'taleinat', 'serhiy.storchaka', 'davin', 'pablogsal']
pr_nums = ['7850', '8500', '9055', '9778']
priority = 'normal'
resolution = None
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue33613'
versions = ['Python 3.8']

[vstinner]

test_semaphore_tracker_sigint() emits a warning. If the test is run with -W error, the test fails.

vstinner@apu$ ./python -m test test_multiprocessing_fork -v -m test_semaphore_tracker_sigint
(...)
test_semaphore_tracker_sigint (test.test_multiprocessing_fork.TestSemaphoreTracker) ... /home/vstinner/prog/python/master/Lib/multiprocessing/semaphore_tracker.py:55: UserWarning: semaphore_tracker: process died unexpectedly, relaunching. Some semaphores might leak.
warnings.warn('semaphore_tracker: process died unexpectedly, '
ok
(...)
Tests result: SUCCESS

vstinner@apu$ ./python -Werror -m test test_multiprocessing_fork -v -m test_semaphore_tracker_sigint
(...)
======================================================================
ERROR: test_semaphore_tracker_sigint (test.test_multiprocessing_fork.TestSemaphoreTracker)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/vstinner/prog/python/master/Lib/test/_test_multiprocessing.py", line 4533, in test_semaphore_tracker_sigint
    self.check_semaphore_tracker_death(signal.SIGINT, False)
  File "/home/vstinner/prog/python/master/Lib/test/_test_multiprocessing.py", line 4521, in check_semaphore_tracker_death
    sem = ctx.Semaphore()
  File "/home/vstinner/prog/python/master/Lib/multiprocessing/context.py", line 82, in Semaphore
    return Semaphore(value, ctx=self.get_context())
  File "/home/vstinner/prog/python/master/Lib/multiprocessing/synchronize.py", line 127, in __init__
    SemLock.__init__(self, SEMAPHORE, value, SEM_VALUE_MAX, ctx=ctx)
  File "/home/vstinner/prog/python/master/Lib/multiprocessing/synchronize.py", line 81, in __init__
    register(self._semlock.name)
  File "/home/vstinner/prog/python/master/Lib/multiprocessing/semaphore_tracker.py", line 83, in register
    self._send('REGISTER', name)
  File "/home/vstinner/prog/python/master/Lib/multiprocessing/semaphore_tracker.py", line 90, in _send
    self.ensure_running()
  File "/home/vstinner/prog/python/master/Lib/multiprocessing/semaphore_tracker.py", line 55, in ensure_running
    warnings.warn('semaphore_tracker: process died unexpectedly, '
UserWarning: semaphore_tracker: process died unexpectedly, relaunching.  Some semaphores might leak.
(...)
Tests result: FAILURE

[vstinner]

Note: I found this bug while working on bpo-33612.

[vstinner]

(Oops, I attached a file and added two comments to this issue, whereas I wanted to comment bpo-33612.)

[serhiy-storchaka]

See also some information about this in bpo-31463.

[pablogsal]

I would like to work on this issue and on bpo-31463

[vstinner]

I would like to work on this issue and on bpo-31463

Say it on bpo-31463 as well on that case ;-)

[pablogsal]

There are two problems in test_semaphore_tracker_sigint. One of the problems is that if should_die is False, the code does not check
that no warning is raised (and the warning is being raised, so this
the error has pass silently.) The other problem is a race condition
between the parent calls _semaphore_tracker.ensure_running() (which in turn spawns the semaphore_tracker using _posixsubprocess.fork_exec) and the child registering the signals. So basically, the parent sends the signal before the child protects against the signal. Modifying:

        _semaphore_tracker.ensure_running()
        pid = _semaphore_tracker._pid
        os.kill(pid, signum)
        time.sleep(1.0)  # give it time to die

to

        _semaphore_tracker.ensure_running()
        pid = _semaphore_tracker._pid
        time.sleep(1.0) # Give time for the child to register the signal handlers
        os.kill(pid, signum)
        time.sleep(1.0)  # give it time to die

fix the issue. I have tested this on one of the most grumpy and slow buildbots (gcc110) and it works.

I cannot think of a different way of waiting for the child to register the signals without modifying the child code for testing so I am not sure that we can do anything rather than the sleep.

[taleinat]

You can know that the semaphore tracker is running by sending it an invalid command and seeing the error written to stderr. The semaphore tracker either inherits sys.stderr from the parent process (if possible) or uses its own. This is a bit of a hack, but might be worth it to avoid another wasted second on every run of the test suite.

[pablogsal]

@taleinat Sadly, if the semaphore_tracker receives an invalid command, it raises and dies, making the test impossible:

                 cmd, name = line.strip().split(b':')
                 if cmd == b'REGISTER':
                     cache.add(name)
                 elif cmd == b'UNREGISTER':
                     cache.remove(name)
                 else:
                     raise RuntimeError('unrecognized command %r' % cmd)

[pitrou]

We can easily add a 'NOOP' command to the semaphore tracker if it helps solve that issue.

[pablogsal]

Sorry, I was wrong, it does not die. I am investigating this solution using stderr.

[pablogsal]

Additionally, as I commented in the PR, apart from the race condition I think the test should be modified so it fails if the warning is raised and not expected:

The problem is that the test was silently failing when SIGINT was >being delivered as the test does not check that no warnings are >raised. Notice that the presence of a warning means that the process >died and therefore SIGINT did kill the process, which is precisely >what the test checks that does not happen.

I think in order to check that the test works as intended without the >need to run the suite with -Wall, we need to modify the test.