From 610289ade4349db93eb2c63331c92f7c22b29bbc Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 19 Feb 2019 08:57:14 -0800 Subject: [PATCH] Revert "[Windows] Fixes lock_and_call test method (#6772)" This reverts commit 0489ca588851430d03c45fecec56fc1ca8223c83. --- certbot/tests/lock_test.py | 2 + certbot/tests/util.py | 93 ++++++++++++++++---------------------- 2 files changed, 41 insertions(+), 54 deletions(-) diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py index 8658443d0..aa1de299b 100644 --- a/certbot/tests/lock_test.py +++ b/certbot/tests/lock_test.py @@ -23,6 +23,7 @@ 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) @@ -53,6 +54,7 @@ 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 953d36536..8c5db2c2f 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -3,6 +3,7 @@ .. warning:: This module is not part of the public API. """ +import multiprocessing import os import pkg_resources import shutil @@ -10,8 +11,6 @@ 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 @@ -24,8 +23,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 configuration from certbot import util +from certbot import configuration from certbot.display import util as display_util @@ -212,7 +211,7 @@ class FreezableMock(object): """ def __init__(self, frozen=False, func=None, return_value=mock.sentinel.DEFAULT): - self._frozen_set = set() if frozen else {'freeze', } + self._frozen_set = set() if frozen else set(('freeze',)) self._func = func self._mock = mock.MagicMock() if return_value != mock.sentinel.DEFAULT: @@ -341,7 +340,6 @@ 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. @@ -360,58 +358,47 @@ 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 """ - 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 to reset internal _LOCKS dictionary reload_module(util) - 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) + # 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() - # 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) + # call func and terminate the child + func() + cv.notify() + cv.release() + child.join() + assert child.exitcode == 0 +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.""" @@ -420,7 +407,6 @@ 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.' @@ -429,10 +415,9 @@ 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)