[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

@@ -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)

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)