Skip to content

bpo-33613, test_semaphore_tracker_sigint: fix race condition #7850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pitrou merged 21 commits into from
Sep 4, 2018
Merged

bpo-33613, test_semaphore_tracker_sigint: fix race condition #7850

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5f1df08
bpo-33613, test_semaphore_tracker_sigint: fix race condition
pablogsal Jun 20, 2018
4f9babb
Implement PONG command in the semaphore tracker
pablogsal Jun 24, 2018
26d81a3
Ensure the tracker is killed to allow multiple testing runs in the sa…
pablogsal Jun 24, 2018
a7d5c59
fixup! Ensure the tracker is killed to allow multiple testing runs in…
pablogsal Jul 1, 2018
a735d49
fixup! fixup! Ensure the tracker is killed to allow multiple testing …
pablogsal Jul 7, 2018
9460da4
fixup! fixup! fixup! Ensure the tracker is killed to allow multiple t…
pablogsal Jul 24, 2018
fb136d0
Use a buffered writer to answer to the PING command
pablogsal Jul 24, 2018
f3d0f0b
Use the tracker own stderr for answering the PING command
pablogsal Jul 24, 2018
f5e5cf9
Use pthread_sigmask to avoid the race condition
pablogsal Jul 27, 2018
4393021
Add comment explaining the race condition
pablogsal Jul 27, 2018
d501f2f
Restore timeout and use os.waitpid
pablogsal Jul 31, 2018
9e0243e
fixup! Restore timeout and use os.waitpid
pablogsal Jul 31, 2018
26e077b
Add News entry
pablogsal Jul 31, 2018
a50912b
fixup! fixup! Restore timeout and use os.waitpid
pablogsal Jul 31, 2018
bcbb942
fixup! Add News entry
pablogsal Jul 31, 2018
0b46f07
fixup! fixup! fixup! Restore timeout and use os.waitpid
pablogsal Aug 1, 2018
66ef7dc
Unregister sigmask if process fails to start
pablogsal Aug 3, 2018
f7d2d4d
Include the signal registering in the try block
pablogsal Sep 3, 2018
4a09fde
Simplify comment when handling ChildProcessError
pablogsal Sep 3, 2018
c9f96c1
Eliminate hardcoded reference to the bug tracker url
pablogsal Sep 3, 2018
c0a088c
Remove extraneous NEWS file
pitrou Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
bpo-33613, test_semaphore_tracker_sigint: fix race condition 
Fail `test_semaphore_tracker_sigint` if no warnings are expected and
one is received.

Fix race condition when the child receives SIGINT
before it can register signal handlers for it.

The race condition occurs when the parent calls
`_semaphore_tracker.ensure_running()` (which in turn spawns the
semaphore_tracker using `_posixsubprocess.fork_exec`), the child
registers the signal handlers and the parent tries to kill the child.
What seem to happen is that in some slow systems, the parent sends the
signal to kill the child before the child protects against the signal.

There is no reliable and portable solution for the parent to wait until
the child has register the signal handlers to send the signal to kill
the child so a `sleep` is introduced between the spawning of the child
and the parent sending the signal to give time to the child to register
the handlers.
  • Loading branch information
@pablogsal
pablogsal committed Jun 21, 2018
commit 5f1df0887c1daefd137e66a90943d2afa292e8fb
16 changes: 11 additions & 5 deletions Lib/test/_test_multiprocessing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import struct
import operator
import weakref
import warnings
import test.support
import test.support.script_helper
from test import support
Expand Down Expand Up @@ -4474,15 +4475,12 @@ def check_semaphore_tracker_death(self, signum, should_die):
from multiprocessing.semaphore_tracker import _semaphore_tracker
_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

@vstinner vstinner Jul 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you reduce time?

@pablogsal pablogsal Jul 27, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because one second seems a lot to me for the signal to be delivered (checked in the buildbot) and this will make the test suite run a bit faster. I can undo this if we want to be conservative :).

@vstinner vstinner Jul 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep 1 second. While you saw the test passing, I'm sure that reducing the sleep will make the test failure more likely on some very busy buildbots.

ctx = multiprocessing.get_context("spawn")
with contextlib.ExitStack() as stack:
if should_die:
stack.enter_context(self.assertWarnsRegex(
UserWarning,
"semaphore_tracker: process died"))
with warnings.catch_warnings(record=True) as all_warn:
sem = ctx.Semaphore()
sem.acquire()
sem.release()
Expand All @@ -4492,6 +4490,14 @@ def check_semaphore_tracker_death(self, signum, should_die):
del sem
gc.collect()
self.assertIsNone(wr())
if should_die:
self.assertEqual(len(all_warn), 1)
the_warn = all_warn[0]
issubclass(the_warn.category, UserWarning)
Comment thread
serhiy-storchaka marked this conversation as resolved.
self.assertTrue("semaphore_tracker: process died"
in str(the_warn.message))
else:
self.assertEqual(len(all_warn), 0)

def test_semaphore_tracker_sigint(self):
# Catchable signal (ignored by semaphore tracker)
Expand Down