From e40d929e804137566c0e04f87f9d5c62a7966a4a Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 15 Feb 2019 01:55:27 +0100 Subject: [PATCH] [Windows|Unix] New platform independent locking mechanism - the revenge (#6663) First PR about this issue, #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 | 56 ---------- certbot/lock.py | 213 +++++++++++++++++++++++++++++-------- certbot/tests/lock_test.py | 57 ++++++---- certbot/tests/util_test.py | 16 +-- 4 files changed, 216 insertions(+), 126 deletions(-) diff --git a/certbot/compat.py b/certbot/compat.py index 3b5d068a6..f533f5954 100644 --- a/certbot/compat.py +++ b/certbot/compat.py @@ -13,13 +13,6 @@ import stat 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': diff --git a/certbot/lock.py b/certbot/lock.py index 3ff46518d..760a12b8f 100644 --- a/certbot/lock.py +++ b/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. - - :param str path: path to the file to lock - - :raises errors.LockError: if unable to acquire the lock - + # 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 """ - super(LockFile, self).__init__() self._path = path - self._fd = None + mechanism = _UnixLockMechanism if POSIX_MODE else _WindowsLockMechanism + self._lock_mechanism = mechanism(path) 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): - """Acquire the lock file. - - :raises errors.LockError: if lock is already held - :raises OSError: if unable to open or stat the lock file - + # 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 + """ + Release the lock on the file, allowing any other Certbot instance to acquire it. + """ + self._lock_mechanism.release() + + 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() + + +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 @@ class LockFile(object): 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 @@ class LockFile(object): # 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: + 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): - """Remove, close, and release the lock file.""" + """Release the lock.""" try: - compat.release_locked_file(self._fd, self._path) + 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 diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py index aa82701f3..aa1de299b 100644 --- a/certbot/tests/lock_test.py +++ b/certbot/tests/lock_test.py @@ -3,6 +3,12 @@ import functools import multiprocessing import os import unittest +try: + import fcntl # pylint: disable=import-error,unused-import +except ImportError: + POSIX_MODE = False +else: + POSIX_MODE = True import mock @@ -10,7 +16,6 @@ from certbot import errors from certbot.tests import util as test_util -@test_util.broken_on_windows class LockDirTest(test_util.TempDirTestCase): """Tests for certbot.lock.lock_dir.""" @classmethod @@ -18,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) @@ -25,7 +31,6 @@ class LockDirTest(test_util.TempDirTestCase): test_util.lock_and_call(assert_raises, lock_path) -@test_util.broken_on_windows class LockFileTest(test_util.TempDirTestCase): """Tests for certbot.lock.LockFile.""" @classmethod @@ -49,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) @@ -71,6 +77,8 @@ class LockFileTest(test_util.TempDirTestCase): self.assertTrue(lock_file.__class__.__name__ in lock_repr) self.assertTrue(self.lock_path in lock_repr) + @test_util.skip_on_windows( + 'Race conditions on lock are specific to the non-blocking file access approach on Linux.') def test_race(self): should_delete = [True, False] stat = os.stat @@ -91,27 +99,36 @@ class LockFileTest(test_util.TempDirTestCase): lock_file.release() self.assertFalse(os.path.exists(self.lock_path)) - @mock.patch('certbot.compat.fcntl.lockf') - def test_unexpected_lockf_err(self, mock_lockf): + def test_unexpected_lockf_or_locking_err(self): + if POSIX_MODE: + mocked_function = 'certbot.lock.fcntl.lockf' + else: + mocked_function = 'certbot.lock.msvcrt.locking' msg = 'hi there' - mock_lockf.side_effect = IOError(msg) - try: - self._call(self.lock_path) - except IOError as err: - self.assertTrue(msg in str(err)) - else: # pragma: no cover - self.fail('IOError not raised') + with mock.patch(mocked_function) as mock_lock: + mock_lock.side_effect = IOError(msg) + try: + self._call(self.lock_path) + except IOError as err: + self.assertTrue(msg in str(err)) + else: # pragma: no cover + self.fail('IOError not raised') - @mock.patch('certbot.lock.os.stat') - def test_unexpected_stat_err(self, mock_stat): + def test_unexpected_os_err(self): + if POSIX_MODE: + mock_function = 'certbot.lock.os.stat' + else: + mock_function = 'certbot.lock.msvcrt.locking' + # The only expected errno are ENOENT and EACCES in lock module. msg = 'hi there' - mock_stat.side_effect = OSError(msg) - try: - self._call(self.lock_path) - except OSError as err: - self.assertTrue(msg in str(err)) - else: # pragma: no cover - self.fail('OSError not raised') + with mock.patch(mock_function) as mock_os: + mock_os.side_effect = OSError(msg) + try: + self._call(self.lock_path) + except OSError as err: + self.assertTrue(msg in str(err)) + else: # pragma: no cover + self.fail('OSError not raised') if __name__ == "__main__": diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 6685b88c6..c85009c62 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -2,7 +2,6 @@ import argparse import errno import os -import shutil import unittest import mock @@ -88,7 +87,6 @@ class LockDirUntilExit(test_util.TempDirTestCase): import certbot.util reload_module(certbot.util) - @test_util.broken_on_windows @mock.patch('certbot.util.logger') @mock.patch('certbot.util.atexit_register') def test_it(self, mock_register, mock_logger): @@ -100,11 +98,15 @@ class LockDirUntilExit(test_util.TempDirTestCase): self.assertEqual(mock_register.call_count, 1) registered_func = mock_register.call_args[0][0] - shutil.rmtree(subdir) - registered_func() # exception not raised - # logger.debug is only called once because the second call - # to lock subdir was ignored because it was already locked - self.assertEqual(mock_logger.debug.call_count, 1) + + from certbot import util + # Despite lock_dir_until_exit has been called twice to subdir, its lock should have been + # added only once. So we expect to have two lock references: for self.tempdir and subdir + self.assertTrue(len(util._LOCKS) == 2) # pylint: disable=protected-access + registered_func() # Exception should not be raised + # Logically, logger.debug, that would be invoked in case of unlock failure, + # should never been called. + self.assertEqual(mock_logger.debug.call_count, 0) class SetUpCoreDirTest(test_util.TempDirTestCase):