From 0489ca588851430d03c45fecec56fc1ca8223c83 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Sat, 16 Feb 2019 03:51:22 +0100 Subject: [PATCH] [Windows] Fixes lock_and_call test method (#6772) The method `lock_and_call`, in `certbot.tests.util` is designed to acquire a lock on a foreign process, then execute a callable in the current process. This is done to closely reproduce the lock mechanism involved between two certbot instances that are running in parallel. This method uses the `multiprocessing` module. But its implementation in `lock_and_call` is broken for Windows: the two processes fail to communicate, leading to a deadlock. In fact, `multiprocessing` module is using the fork mechanism on Linux, and the spawn mechanism on Windows, leading to behavior inconsistencies between the two platforms. As this method is for tests, and not for production code, I did not try to make two implementations "by the book", one suitable for Windows, the other for Linux, like for the `certbot.lock` module. Instead, I use a `subprocess` approach with a trigger file allowing to coordinate the current process and the subprocess. With this, `lock_and_call` is running from the same code both on Linux and Windows. Relevant tests in the `certbot.tests.lock_test` test module are now enabled for Windows. * Implement new lock_and_call method * Reactivate tests for Windows --- certbot/tests/lock_test.py | 2 - certbot/tests/util.py | 93 ++++++++++++++++++++++---------------- 2 files changed, 54 insertions(+), 41 deletions(-) diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py index aa1de299b..8658443d0 100644 --- a/certbot/tests/lock_test.py +++ b/certbot/tests/lock_test.py @@ -23,7 +23,6 @@ class LockDirTest(test_util.TempDirTestCase): from certbot.lock import lock_dir return lock_dir(*args, **kwargs) - @test_util.broken_on_windows def test_it(self): assert_raises = functools.partial( self.assertRaises, errors.LockError, self._call, self.tempdir) @@ -54,7 +53,6 @@ class LockFileTest(test_util.TempDirTestCase): # Test we're still able to properly acquire and release the lock self.test_removed() - @test_util.broken_on_windows def test_contention(self): assert_raises = functools.partial( self.assertRaises, errors.LockError, self._call, self.lock_path) diff --git a/certbot/tests/util.py b/certbot/tests/util.py index 8c5db2c2f..953d36536 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -3,7 +3,6 @@ .. warning:: This module is not part of the public API. """ -import multiprocessing import os import pkg_resources import shutil @@ -11,6 +10,8 @@ import tempfile import unittest import sys import warnings +import subprocess +import time from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization @@ -23,8 +24,8 @@ from six.moves import reload_module # pylint: disable=import-error from certbot import constants from certbot import interfaces from certbot import storage -from certbot import util from certbot import configuration +from certbot import util from certbot.display import util as display_util @@ -211,7 +212,7 @@ class FreezableMock(object): """ def __init__(self, frozen=False, func=None, return_value=mock.sentinel.DEFAULT): - self._frozen_set = set() if frozen else set(('freeze',)) + self._frozen_set = set() if frozen else {'freeze', } self._func = func self._mock = mock.MagicMock() if return_value != mock.sentinel.DEFAULT: @@ -340,6 +341,7 @@ class TempDirTestCase(unittest.TestCase): warnings.warn(message) shutil.rmtree(self.tempdir, onerror=onerror_handler) + class ConfigTestCase(TempDirTestCase): """Test class which sets up a NamespaceConfig object. @@ -358,47 +360,58 @@ class ConfigTestCase(TempDirTestCase): self.config.chain_path = constants.CLI_DEFAULTS['auth_chain_path'] self.config.server = "https://example.com" -def lock_and_call(func, lock_path): - """Grab a lock for lock_path and call func. - - :param callable func: object to call after acquiring the lock - :param str lock_path: path to file or directory to lock +def lock_and_call(callback, path_to_lock): + """Grab a lock on path_to_lock from a foreign process and call the callback. + :param callable callback: object to call after acquiring the lock + :param str path_to_lock: path to file or directory to lock """ - # Reload module to reset internal _LOCKS dictionary + script = """\ +import os +import sys +import time +from certbot import lock + +path_to_lock = sys.argv[1] +trigger = sys.argv[2] + +if os.path.isdir(path_to_lock): + my_lock = lock.lock_dir(path_to_lock) +else: + my_lock = lock.LockFile(path_to_lock) +try: + open(trigger, 'w').close() + while os.path.exists(trigger): + time.sleep(1) +finally: + my_lock.release() +""" + # Reload certbot.util module to reset internal _LOCKS dictionary. reload_module(util) - # start child and wait for it to grab the lock - cv = multiprocessing.Condition() - cv.acquire() - child_args = (cv, lock_path,) - child = multiprocessing.Process(target=hold_lock, args=child_args) - child.start() - cv.wait() + workspace = tempfile.mkdtemp() + try: + tmp_script = os.path.join(workspace, 'test_script.py') + with open(tmp_script, 'w') as file_handle: + file_handle.write(script) - # call func and terminate the child - func() - cv.notify() - cv.release() - child.join() - assert child.exitcode == 0 + # Trigger file is used to coordinate current process and its subprocess. + trigger = os.path.join(workspace, 'trigger') + process = subprocess.Popen([sys.executable, tmp_script, path_to_lock, trigger]) + try: + # Poll and wait for the lock to be acquired, spotted by the trigger file creation. + while not os.path.exists(trigger): + time.sleep(1) + # Then execute the callback. + callback() + finally: + # This will trigger the lock release in subprocess. + os.remove(trigger) + process.communicate() + assert process.returncode == 0 + finally: + shutil.rmtree(workspace) -def hold_lock(cv, lock_path): # pragma: no cover - """Acquire a file lock at lock_path and wait to release it. - - :param multiprocessing.Condition cv: condition for synchronization - :param str lock_path: path to the file lock - - """ - from certbot import lock - if os.path.isdir(lock_path): - my_lock = lock.lock_dir(lock_path) - else: - my_lock = lock.LockFile(lock_path) - cv.acquire() - cv.notify() - cv.wait() - my_lock.release() def skip_on_windows(reason): """Decorator to skip permanently a test on Windows. A reason is required.""" @@ -407,6 +420,7 @@ def skip_on_windows(reason): return unittest.skipIf(sys.platform == 'win32', reason)(function) return wrapper + def broken_on_windows(function): """Decorator to skip temporarily a broken test on Windows.""" reason = 'Test is broken and ignored on windows but should be fixed.' @@ -415,9 +429,10 @@ def broken_on_windows(function): and os.environ.get('SKIP_BROKEN_TESTS_ON_WINDOWS', 'true') == 'true', reason)(function) + def temp_join(path): """ Return the given path joined to the tempdir path for the current platform Eg.: 'cert' => /tmp/cert (Linux) or 'C:\\Users\\currentuser\\AppData\\Temp\\cert' (Windows) """ - return os.path.join(tempfile.gettempdir(), path) + return os.path.join(tempfile.gettempdir(), path)