[Windows|Unix] New platform independent locking mechanism - the reven…

…ge (certbot#6663)

First PR about this issue, certbot#6440, involved to much refactoring to ensure a correct behavior on Linux and installer plugins.

This PR proposes a new implementation of a lock mechanism for Linux and Windows, without implying a refactoring. It takes strictly the existing behavior for Linux, and add the appropriate logic for Windows. The `lock` module formalizes two independant mechanism dedicated to each platform, to improve maintainability.

Tests related to locking are re-activated for Windows, or definitively skipped because of irrelevancy for this platform. 6 more tests are enabled overall.

* Reimplement lock file on basic level

* Remove unused code

* Re-activate some tests

* Update doc

* Reactivate tests relevant to locks in Windows. Correct a test that was not testing what is supposed to test.

* Clean compat.

* Move close sooner in Windows lock implementation

* Add strong mypy types

* Use os.name

* Refactor lock mechanism logic

* Enable more tests

* Update lock.py

* Update lock_test.py

certbot/compat.py

@@ -13,13 +13,6 @@
 
 from certbot import errors
 
-try:
-    # Linux specific
-    import fcntl # pylint: disable=import-error
-except ImportError:
-    # Windows specific
-    import msvcrt # pylint: disable=import-error
-
 UNPRIVILEGED_SUBCOMMANDS_ALLOWED = [
     'certificates', 'enhance', 'revoke', 'delete',
     'register', 'unregister', 'config_changes', 'plugins']
@@ -118,55 +111,6 @@ def readline_with_timeout(timeout, prompt):
         return sys.stdin.readline()
 
 
-def lock_file(fd):
-    """
-    Lock the file linked to the specified file descriptor.
-
-    :param int fd: The file descriptor of the file to lock.
-
-    """
-    if 'fcntl' in sys.modules:
-        # Linux specific
-        fcntl.lockf(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
-    else:
-        # Windows specific
-        msvcrt.locking(fd, msvcrt.LK_NBLCK, 1)
-
-
-def release_locked_file(fd, path):
-    """
-    Remove, close, and release a lock file specified by its file descriptor and its path.
-
-    :param int fd: The file descriptor of the lock file.
-    :param str path: The path of the lock file.
-
-    """
-    # Linux specific
-    #
-    # It is important the lock file is removed before it's released,
-    # otherwise:
-    #
-    # process A: open lock file
-    # process B: release lock file
-    # process A: lock file
-    # process A: check device and inode
-    # process B: delete file
-    # process C: open and lock a different file at the same path
-    try:
-        os.remove(path)
-    except OSError as err:
-        if err.errno == errno.EACCES:
-            # Windows specific
-            # We will not be able to remove a file before closing it.
-            # To avoid race conditions described for Linux, we will not delete the lockfile,
-            # just close it to be reused on the next Certbot call.
-            pass
-        else:
-            raise
-    finally:
-        os.close(fd)
-
-
 def compare_file_modes(mode1, mode2):
     """Return true if the two modes can be considered as equals for this platform"""
     if os.name != 'nt':

certbot/lock.py

@@ -1,15 +1,23 @@
-"""Implements file locks for locking files and directories in UNIX."""
+"""Implements file locks compatible with Linux and Windows for locking files and directories."""
 import errno
 import logging
 import os
+try:
+    import fcntl  # pylint: disable=import-error
+except ImportError:
+    import msvcrt  # pylint: disable=import-error
+    POSIX_MODE = False
+else:
+    POSIX_MODE = True
 
-from certbot import compat
 from certbot import errors
+from acme.magic_typing import Optional, Callable  # pylint: disable=unused-import, no-name-in-module
 
 logger = logging.getLogger(__name__)
 
 
 def lock_dir(dir_path):
+    # type: (str) -> LockFile
     """Place a lock file on the directory at dir_path.
 
     The lock file is placed in the root of dir_path with the name
@@ -27,34 +35,99 @@ def lock_dir(dir_path):
 
 
 class LockFile(object):
-    """A UNIX lock file.
-
-    This lock file is released when the locked file is closed or the
-    process exits. It cannot be used to provide synchronization between
-    threads. It is based on the lock_file package by Martin Horcicka.
-
+    """
+    Platform independent file lock system.
+    LockFile accepts a parameter, the path to a file acting as a lock. Once the LockFile,
+    instance is created, the associated file is 'locked from the point of view of the OS,
+    meaning that if another instance of Certbot try at the same time to acquire the same lock,
+    it will raise an Exception. Calling release method will release the lock, and make it
+    available to every other instance.
+    Upon exit, Certbot will also release all the locks.
+    This allows us to protect a file or directory from being concurrently accessed
+    or modified by two Certbot instances.
+    LockFile is platform independent: it will proceed to the appropriate OS lock mechanism
+    depending on Linux or Windows.
     """
     def __init__(self, path):
-        """Initialize and acquire the lock file.
+        # type: (str) -> None
+        """
+        Create a LockFile instance on the given file path, and acquire lock.
+        :param str path: the path to the file that will hold a lock
+        """
+        self._path = path
+        mechanism = _UnixLockMechanism if POSIX_MODE else _WindowsLockMechanism
+        self._lock_mechanism = mechanism(path)
 
-        :param str path: path to the file to lock
+        self.acquire()
 
+    def __repr__(self):
+        # type: () -> str
+        repr_str = '{0}({1}) <'.format(self.__class__.__name__, self._path)
+        if self.is_locked():
+            repr_str += 'acquired>'
+        else:
+            repr_str += 'released>'
+        return repr_str
+
+    def acquire(self):
+        # type: () -> None
+        """
+        Acquire the lock on the file, forbidding any other Certbot instance to acquire it.
         :raises errors.LockError: if unable to acquire the lock
+        """
+        self._lock_mechanism.acquire()
 
+    def release(self):
+        # type: () -> None
         """
-        super(LockFile, self).__init__()
-        self._path = path
-        self._fd = None
+        Release the lock on the file, allowing any other Certbot instance to acquire it.
+        """
+        self._lock_mechanism.release()
 
-        self.acquire()
+    def is_locked(self):
+        # type: () -> bool
+        """
+        Check if the file is currently locked.
+        :return: True if the file is locked, False otherwise
+        """
+        return self._lock_mechanism.is_locked()
 
-    def acquire(self):
-        """Acquire the lock file.
 
-        :raises errors.LockError: if lock is already held
-        :raises OSError: if unable to open or stat the lock file
+class _BaseLockMechanism(object):
+    def __init__(self, path):
+        # type: (str) -> None
+        """
+        Create a lock file mechanism for Unix.
+        :param str path: the path to the lock file
+        """
+        self._path = path
+        self._fd = None  # type: Optional[int]
 
+    def is_locked(self):
+        # type: () -> bool
+        """Check if lock file is currently locked.
+        :return: True if the lock file is locked
+        :rtype: bool
         """
+        return self._fd is not None
+
+    def acquire(self):  # pylint: disable=missing-docstring
+        pass  # pragma: no cover
+
+    def release(self):  # pylint: disable=missing-docstring
+        pass  # pragma: no cover
+
+
+class _UnixLockMechanism(_BaseLockMechanism):
+    """
+    A UNIX lock file mechanism.
+    This lock file is released when the locked file is closed or the
+    process exits. It cannot be used to provide synchronization between
+    threads. It is based on the lock_file package by Martin Horcicka.
+    """
+    def acquire(self):
+        # type: () -> None
+        """Acquire the lock."""
         while self._fd is None:
             # Open the file
             fd = os.open(self._path, os.O_CREAT | os.O_WRONLY, 0o600)
@@ -68,33 +141,29 @@ def acquire(self):
                     os.close(fd)
 
     def _try_lock(self, fd):
-        """Try to acquire the lock file without blocking.
-
+        # type: (int) -> None
+        """
+        Try to acquire the lock file without blocking.
         :param int fd: file descriptor of the opened file to lock
-
         """
         try:
-            compat.lock_file(fd)
+            fcntl.lockf(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
         except IOError as err:
             if err.errno in (errno.EACCES, errno.EAGAIN):
-                logger.debug(
-                    "A lock on %s is held by another process.", self._path)
-                raise errors.LockError(
-                    "Another instance of Certbot is already running.")
+                logger.debug('A lock on %s is held by another process.', self._path)
+                raise errors.LockError('Another instance of Certbot is already running.')
             raise
 
     def _lock_success(self, fd):
-        """Did we successfully grab the lock?
-
+        # type: (int) -> bool
+        """
+        Did we successfully grab the lock?
         Because this class deletes the locked file when the lock is
         released, it is possible another process removed and recreated
         the file between us opening the file and acquiring the lock.
-
         :param int fd: file descriptor of the opened file to lock
-
         :returns: True if the lock was successfully acquired
         :rtype: bool
-
         """
         try:
             stat1 = os.stat(self._path)
@@ -108,17 +177,75 @@ def _lock_success(self, fd):
         # the same device and inode, they're the same file.
         return stat1.st_dev == stat2.st_dev and stat1.st_ino == stat2.st_ino
 
-    def __repr__(self):
-        repr_str = '{0}({1}) <'.format(self.__class__.__name__, self._path)
-        if self._fd is None:
-            repr_str += 'released>'
-        else:
-            repr_str += 'acquired>'
-        return repr_str
-
     def release(self):
+        # type: () -> None
         """Remove, close, and release the lock file."""
+        # It is important the lock file is removed before it's released,
+        # otherwise:
+        #
+        # process A: open lock file
+        # process B: release lock file
+        # process A: lock file
+        # process A: check device and inode
+        # process B: delete file
+        # process C: open and lock a different file at the same path
+        try:
+            os.remove(self._path)
+        finally:
+            # Following check is done to make mypy happy: it ensure that self._fd, marked
+            # as Optional[int] is effectively int to make it compatible with os.close signature.
+            if self._fd is None:  # pragma: no cover
+                raise TypeError('Error, self._fd is None.')
+            try:
+                os.close(self._fd)
+            finally:
+                self._fd = None
+
+
+class _WindowsLockMechanism(_BaseLockMechanism):
+    """
+    A Windows lock file mechanism.
+    By default on Windows, acquiring a file handler gives exclusive access to the process
+    and results in an effective lock. However, it is possible to explicitly acquire the
+    file handler in shared access in terms of read and write, and this is done by os.open
+    and io.open in Python. So an explicit lock needs to be done through the call of
+    msvcrt.locking, that will lock the first byte of the file. In theory, it is also
+    possible to access a file in shared delete access, allowing other processes to delete an
+    opened file. But this needs also to be done explicitly by all processes using the Windows
+    low level APIs, and Python does not do it. As of Python 3.7 and below, Python developers
+    state that deleting a file opened by a process from another process is not possible with
+    os.open and io.open.
+    Consequently, mscvrt.locking is sufficient to obtain an effective lock, and the race
+    condition encountered on Linux is not possible on Windows, leading to a simpler workflow.
+    """
+    def acquire(self):
+        """Acquire the lock"""
+        open_mode = os.O_RDWR | os.O_CREAT | os.O_TRUNC
+
+        fd = os.open(self._path, open_mode, 0o600)
         try:
-            compat.release_locked_file(self._fd, self._path)
+            msvcrt.locking(fd, msvcrt.LK_NBLCK, 1)
+        except (IOError, OSError) as err:
+            os.close(fd)
+            # Anything except EACCES is unexpected. Raise directly the error in that case.
+            if err.errno != errno.EACCES:
+                raise
+            logger.debug('A lock on %s is held by another process.', self._path)
+            raise errors.LockError('Another instance of Certbot is already running.')
+
+        self._fd = fd
+
+    def release(self):
+        """Release the lock."""
+        try:
+            msvcrt.locking(self._fd, msvcrt.LK_UNLCK, 1)
+            os.close(self._fd)
+
+            try:
+                os.remove(self._path)
+            except OSError as e:
+                # If the lock file cannot be removed, it is not a big deal.
+                # Likely another instance is acquiring the lock we just released.
+                logger.debug(str(e))
         finally:
             self._fd = None