diff --git a/etc/spack/defaults/windows/config.yaml b/etc/spack/defaults/windows/config.yaml index f54febe957553e..af90cc33b75c8d 100644 --- a/etc/spack/defaults/windows/config.yaml +++ b/etc/spack/defaults/windows/config.yaml @@ -1,5 +1,5 @@ config: - locks: false + locks: true build_stage:: - '$user_cache_path/stage' stage_name: '{name}-{version}-{hash:7}' diff --git a/lib/spack/spack/llnl/util/lock.py b/lib/spack/spack/llnl/util/lock.py index 63abafa6ece8aa..6253c81ca281b7 100644 --- a/lib/spack/spack/llnl/util/lock.py +++ b/lib/spack/spack/llnl/util/lock.py @@ -2,21 +2,38 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import contextlib import errno import os import socket -import sys import time from datetime import datetime +from sys import platform as _platform from types import TracebackType -from typing import IO, Any, Callable, ContextManager, Dict, Generator, Optional, Tuple, Type, Union +from typing import ( # novm + IO, + Any, + Callable, + ContextManager, + Dict, + Generator, + Optional, + Tuple, + Type, + Union, +) from spack.llnl.util import lang, tty from ..string import plural -if sys.platform != "win32": +IS_WINDOWS = _platform == "win32" +if not IS_WINDOWS: import fcntl +else: + import pywintypes + import win32con + import win32file __all__ = [ @@ -33,6 +50,8 @@ "CantCreateLockError", ] +WHOLE_FILE_RANGE = 0xFFFFFFFF if IS_WINDOWS else 0 + ReleaseFnType = Optional[Callable[[], bool]] @@ -172,9 +191,106 @@ def _attempts_str(wait_time, nattempts): return " after {} and {}".format(lang.pretty_seconds(wait_time), attempts) +def _low_high(value): + low = value & 0xFFFFFFFF + high = (value >> 32) & 0xFFFFFFFF + return low, high + + +def _setup_overlapped(offset): + overlapped = pywintypes.OVERLAPPED() + # hEvent needs to be null per lockfileex docs + overlapped.hEvent = 0 + offset_low, offset_high = _low_high(offset) + overlapped.Offset = offset_low + overlapped.OffsetHigh = offset_high + return overlapped + + +@contextlib.contextmanager +def _safe_exclusion(lock: "Lock", timeout: Optional[float] = None): + """Lock upgrade guard for Windows, designed to allow for lock upgrading + which Windows file locks do not natively support + Uses one additional lockfile as a "gate" for upgrades. + + If a process is attempting to upgrade from a read to a write + it must first take a write lock on this intermediate file + before releasing existing read lock and retaking a lock on the same + file but exclusively. + This gate file prevents any other process/lock from catching + the lock with a competing write during the release part of the upgrade. + """ + # probably something like foo.lock.lock + timeout = timeout or lock.default_timeout + lock_path = lock.path + ".lock" + lk = Lock(lock_path, start=lock._start, length=lock._length) + # cache previous value for read locks + _reads = lock._reads + try: + # don't use the acquire lock method to avoid + # recursion + lk._lock(LockType.WRITE, timeout=timeout) + # lock gate acquired, drop read lock if there is one + # cannot use release read as we need to drop + # the read lock if there is one, not just decrement the nested + # lock tracker + if _reads: + lock._release_lock() + yield + except LockError: + # doesn't matter what the error was + # if there was a read lock previously + # restore it + if _reads > 0: + lock._lock(LockType.READ, timeout=timeout) + raise + finally: + if lk.is_write_locked(): + lk.release_write() + + +@contextlib.contextmanager +def _safe_downgrade(lock: "Lock", timeout: Optional[float] = None): + """Windows can hold shared and exclusive locks on the same time, but only + if the exclusive lock is held first. When the shared lock overlaps the exclusive + range, the exclusive lock must also be unlocked and must be unlocked *first* + From the MSFT docs: + + A shared lock can overlap an exclusive lock if both locks were created using + the same file handle. If the same range is locked with an exclusive and a + shared lock, two unlock operations are necessary to unlock the region; the + first unlock operation unlocks the exclusive lock, the second unlock + operation unlocks the shared lock. + + Since we are "downgrading" we no longer care to have an exclusive lock + so we just give it up + """ + timeout = timeout or lock.default_timeout + # release the read lock first + # so in the event on an error, we still have the exclusive lock + yield + # From the docstring: + # first unlock operation unlocks the exclusive lock, the second unlock + # operation unlocks the shared lock. + # + # This will remove the exclusive lock + lock._release_lock() + + class LockType: READ = 0 WRITE = 1 + if IS_WINDOWS: + LOCK_EX = win32con.LOCKFILE_EXCLUSIVE_LOCK # exclusive lock + LOCK_SH = 0 # shared lock, default + LOCK_NB = win32con.LOCKFILE_FAIL_IMMEDIATELY # non-blocking + LOCK_CATCH = pywintypes.error + else: + LOCK_EX = fcntl.LOCK_EX + LOCK_SH = fcntl.LOCK_SH + LOCK_NB = fcntl.LOCK_NB + LOCK_UN = fcntl.LOCK_UN + LOCK_CATCH = IOError @staticmethod def to_str(tid): @@ -185,9 +301,9 @@ def to_str(tid): @staticmethod def to_module(tid): - lock = fcntl.LOCK_SH + lock = LockType.LOCK_SH if tid == LockType.WRITE: - lock = fcntl.LOCK_EX + lock = LockType.LOCK_EX return lock @staticmethod @@ -215,7 +331,7 @@ def __init__( path: str, *, start: int = 0, - length: int = 0, + length: int = WHOLE_FILE_RANGE, default_timeout: Optional[float] = None, debug: bool = False, desc: str = "", @@ -243,6 +359,7 @@ def __init__( """ self.path = path self._file: Optional[IO[bytes]] = None + self._file_mode = "" self._reads = 0 self._writes = 0 @@ -267,6 +384,18 @@ def __init__( self.host: Optional[str] = None self.old_host: Optional[str] = None + self._current_lock = None + + def __lock_fail_condition(self, e): + if IS_WINDOWS: + # 33 "The process cannot access the file because another + # process has locked a portion of the file." + # 32 "The process cannot access the file because it is being + # used by another process" + return e.args[0] not in (32, 33) + else: + return e.errno not in (errno.EAGAIN, errno.EACCES) + @staticmethod def _poll_interval_generator( _wait_times: Optional[Tuple[float, float, float]] = None, @@ -298,6 +427,8 @@ def __repr__(self) -> str: rep = "{0}(".format(self.__class__.__name__) for attr, value in self.__dict__.items(): rep += "{0}={1}, ".format(attr, value.__repr__()) + if attr != "LOCK_CATCH": + rep += "{0}={1}, ".format(attr, value.__repr__()) return "{0})".format(rep.strip(", ")) def __str__(self) -> str: @@ -328,7 +459,7 @@ def _lock(self, op: int, timeout: Optional[float] = None) -> Tuple[float, int]: self._ensure_parent_directory() self._file = FILE_TRACKER.get_fh(self.path) - if LockType.to_module(op) == fcntl.LOCK_EX and self._file.mode == "rb": + if LockType.to_module(op) == LockType.LOCK_EX and self._file.mode == "rb": # Attempt to upgrade to write lock w/a read-only file. # If the file were writable, we'd have opened it rb+ raise LockROFileError(self.path) @@ -362,15 +493,24 @@ def _poll_lock(self, op: int) -> bool: """ assert self._file is not None, "cannot poll a lock without the file being set" module_op = LockType.to_module(op) + try: - # Try to get the lock (will raise if not available.) - fcntl.lockf( - self._file.fileno(), - module_op | fcntl.LOCK_NB, - self._length, - self._start, - os.SEEK_SET, - ) + if IS_WINDOWS: + overlapped = _setup_overlapped(self._start) + range_low, range_high = _low_high(self._length) + hfile = win32file._get_osfhandle(self._file.fileno()) + win32file.LockFileEx( + hfile, module_op | LockType.LOCK_NB, range_low, range_high, overlapped # flags + ) + else: + # Try to get the lock (will raise if not available.) + fcntl.lockf( + self._file.fileno(), + module_op | LockType.LOCK_NB, + self._length, + self._start, + os.SEEK_SET, + ) # help for debugging distributed locking if self.debug: @@ -383,7 +523,7 @@ def _poll_lock(self, op: int) -> bool: ) # Exclusive locks write their PID/host - if module_op == fcntl.LOCK_EX: + if op == LockType.WRITE: self._write_log_debug_data() return True @@ -392,12 +532,15 @@ def _poll_lock(self, op: int) -> bool: # EAGAIN and EACCES == locked by another process (so try again) if e.errno not in (errno.EAGAIN, errno.EACCES): raise + except LockType.LOCK_CATCH as e: + # check if lock failure or lock is already held + if self.__lock_fail_condition(e): + raise return False def _ensure_parent_directory(self) -> str: parent = os.path.dirname(self.path) - # relative paths to lockfiles in the current directory have no parent if not parent: return "." @@ -407,6 +550,7 @@ def _ensure_parent_directory(self) -> str: def _read_log_debug_data(self) -> None: """Read PID and host data out of the file if it is there.""" assert self._file is not None, "cannot read debug log without the file being set" + self.old_pid = self.pid self.old_host = self.host @@ -420,12 +564,12 @@ def _read_log_debug_data(self) -> None: def _write_log_debug_data(self) -> None: """Write PID and host data to the file, recording old values.""" assert self._file is not None, "cannot write debug log without the file being set" + self.old_pid = self.pid self.old_host = self.host self.pid = os.getpid() self.host = socket.gethostname() - # write pid, host to disk to sync over FS self._file.seek(0) self._file.write(f"pid={self.pid},host={self.host}".encode("utf-8")) @@ -433,17 +577,32 @@ def _write_log_debug_data(self) -> None: self._file.flush() os.fsync(self._file.fileno()) - def _unlock(self) -> None: + def _release_lock(self): + if IS_WINDOWS: + hfile = win32file._get_osfhandle(self._file.fileno()) + overlapped = _setup_overlapped(self._start) + low_range, high_range = _low_high(self._length) + win32file.UnlockFileEx(hfile, low_range, high_range, overlapped) + else: + fcntl.lockf( + self._file.fileno(), LockType.LOCK_UN, self._length, self._start, os.SEEK_SET + ) + + def _unlock(self): """Releases a lock using POSIX locks (``fcntl.lockf``) Releases the lock regardless of mode. Note that read locks may be masquerading as write locks, but this removes either. + Reset all lock attributes to initial states """ assert self._file is not None, "cannot unlock without the file being set" - fcntl.lockf(self._file.fileno(), fcntl.LOCK_UN, self._length, self._start, os.SEEK_SET) + + self._release_lock() FILE_TRACKER.release_by_fh(self._file) + self._file = None + self._file_mode = "" self._reads = 0 self._writes = 0 @@ -464,6 +623,7 @@ def acquire_read(self, timeout: Optional[float] = None) -> bool: # can raise LockError. wait_time, nattempts = self._lock(LockType.READ, timeout=timeout) self._reads += 1 + self._current_lock = LockType.LOCK_SH # Log if acquired, which includes counts when verbose self._log_acquired("READ LOCK", wait_time, nattempts) return True @@ -486,17 +646,19 @@ def acquire_write(self, timeout: Optional[float] = None) -> bool: timeout = timeout or self.default_timeout if self._writes == 0: - # can raise LockError. - wait_time, nattempts = self._lock(LockType.WRITE, timeout=timeout) - self._writes += 1 - # Log if acquired, which includes counts when verbose - self._log_acquired("WRITE LOCK", wait_time, nattempts) - - # return True only if we weren't nested in a read lock. - # TODO: we may need to return two values: whether we got - # the write lock, and whether this is acquiring a read OR - # write lock for the first time. Now it returns the latter. - return self._reads == 0 + with _safe_exclusion(self): + # can raise LockError. + wait_time, nattempts = self._lock(LockType.WRITE, timeout=timeout) + self._writes += 1 + self._current_lock = LockType.LOCK_EX + # Log if acquired, which includes counts when verbose + self._log_acquired("WRITE LOCK", wait_time, nattempts) + + # return True only if we weren't nested in a read lock. + # TODO: we may need to return two values: whether we got + # the write lock, and whether this is acquiring a read OR + # write lock for the first time. Now it returns the latter. + return self._reads == 0 else: # Increment the write count for nested lock tracking self._writes += 1 @@ -528,13 +690,14 @@ def downgrade_write_to_read(self, timeout: Optional[float] = None) -> None: """ timeout = timeout or self.default_timeout - if self._writes == 1 and self._reads == 0: - self._log_downgrading() - # can raise LockError. - wait_time, nattempts = self._lock(LockType.READ, timeout=timeout) - self._reads = 1 - self._writes = 0 - self._log_downgraded(wait_time, nattempts) + if self._writes == 1: + with _safe_downgrade(self): + self._log_downgrading() + # can raise LockError. + wait_time, nattempts = self._lock(LockType.READ, timeout=timeout) + self._reads = 1 + self._writes = 0 + self._log_downgraded(wait_time, nattempts) else: raise LockDowngradeError(self.path) @@ -547,13 +710,14 @@ def upgrade_read_to_write(self, timeout: Optional[float] = None) -> None: """ timeout = timeout or self.default_timeout - if self._reads == 1 and self._writes == 0: + if self._reads >= 1 and self._writes == 0: self._log_upgrading() - # can raise LockError. - wait_time, nattempts = self._lock(LockType.WRITE, timeout=timeout) - self._reads = 0 - self._writes = 1 - self._log_upgraded(wait_time, nattempts) + with _safe_exclusion(self): + # can raise LockError. + wait_time, nattempts = self._lock(LockType.WRITE, timeout=timeout) + self._reads = 0 + self._writes = 1 + self._log_upgraded(wait_time, nattempts) else: raise LockUpgradeError(self.path) diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py index 26262132100e26..3c86df6294fa84 100644 --- a/lib/spack/spack/test/llnl/util/lock.py +++ b/lib/spack/spack/test/llnl/util/lock.py @@ -63,8 +63,10 @@ if sys.platform != "win32": import fcntl +else: + import win32file -pytestmark = pytest.mark.not_on_windows("does not run on windows") +# pytestmark = pytest.mark.not_on_windows("does not run on windows") # @@ -293,7 +295,7 @@ def wait(self): # Process snippets below can be composed into tests. # class AcquireWrite: - def __init__(self, lock_path, start=0, length=0): + def __init__(self, lock_path, start=0, length=1): self.lock_path = lock_path self.start = start self.length = length @@ -310,7 +312,7 @@ def __call__(self, barrier): class AcquireRead: - def __init__(self, lock_path, start=0, length=0): + def __init__(self, lock_path, start=0, length=1): self.lock_path = lock_path self.start = start self.length = length @@ -327,7 +329,7 @@ def __call__(self, barrier): class TimeoutWrite: - def __init__(self, lock_path, start=0, length=0): + def __init__(self, lock_path, start=0, length=1): self.lock_path = lock_path self.start = start self.length = length @@ -345,7 +347,7 @@ def __call__(self, barrier): class TimeoutRead: - def __init__(self, lock_path, start=0, length=0): + def __init__(self, lock_path, start=0, length=1): self.lock_path = lock_path self.start = start self.length = length @@ -583,7 +585,7 @@ def test_write_lock_timeout_with_multiple_readers_3_2_ranges(lock_path): ) -@pytest.mark.skipif(getuid() == 0, reason="user is root") +@pytest.mark.skipif(sys.platform == "win32" or getuid() == 0, reason="user is root") def test_read_lock_on_read_only_lockfile(lock_dir, lock_path): """read-only directory, read-only lockfile.""" touch(lock_path) @@ -611,7 +613,8 @@ def test_read_lock_read_only_dir_writable_lockfile(lock_dir, lock_path): pass -@pytest.mark.skipif(False if sys.platform == "win32" else getuid() == 0, reason="user is root") +# skipping on Windows as spack cannot currently make directories read only +@pytest.mark.skipif(sys.platform == "win32" or getuid() == 0, reason="user is root") def test_read_lock_no_lockfile(lock_dir, lock_path): """read-only directory, no lockfile (so can't create).""" with read_only(lock_dir): @@ -1298,7 +1301,7 @@ def test_attempts_str(): def test_lock_str(): lock = lk.Lock("lockfile") lockstr = str(lock) - assert "lockfile[0:0]" in lockstr + assert f"lockfile[0:{lk.WHOLE_FILE_RANGE}]" in lockstr assert "timeout=None" in lockstr assert "#reads=0, #writes=0" in lockstr @@ -1325,6 +1328,7 @@ def test_downgrade_write_fails(tmp_path: pathlib.Path): lock.release_read() +@pytest.mark.skipif(sys.platform == "win32", reason="fcntl unavailable on Windows") @pytest.mark.parametrize( "err_num,err_msg", [ @@ -1347,10 +1351,37 @@ def _lockf(fd, cmd, len, start, whence): monkeypatch.setattr(fcntl, "lockf", _lockf) if err_num in [errno.EAGAIN, errno.EACCES]: - assert not lock._poll_lock(fcntl.LOCK_EX) + assert not lock._poll_lock(lk.LockType.LOCK_EX) + else: + with pytest.raises(OSError, match=err_msg): + lock._poll_lock(lk.LockType.LOCK_EX) + + monkeypatch.undo() + lock.release_read() + + +@pytest.mark.skipif(sys.platform != "win32", reason="win32file only available on Windows") +@pytest.mark.parametrize( + "err_num,err_msg", [(32, "Fake EACCES error analog"), (33, "Fake EAGAIN error analog")] +) +def test_poll_lock_exception_win(tmp_path: pathlib.Path, monkeypatch, err_num, err_msg): + """Test poll lock exception handling.""" + + def LockFileEx(hfile, int_, int1_, int2_, ol): + raise OSError(err_num, err_msg) + + with working_dir(str(tmp_path)): + lockfile = "lockfile" + lock = lk.Lock(lockfile) + lock.acquire_read() + + monkeypatch.setattr(win32file, "LockFileEx", LockFileEx) + + if err_num in [errno.EAGAIN, errno.EACCES]: + assert not lock._poll_lock(lk.LockType.LOCK_EX) else: with pytest.raises(OSError, match=err_msg): - lock._poll_lock(fcntl.LOCK_EX) + lock._poll_lock(lk.LockType.LOCK_EX) monkeypatch.undo() lock.release_read() diff --git a/lib/spack/spack/util/lock.py b/lib/spack/spack/util/lock.py index e407f7099cbd67..0f085a3c05759b 100644 --- a/lib/spack/spack/util/lock.py +++ b/lib/spack/spack/util/lock.py @@ -5,7 +5,6 @@ """Wrapper for ``spack.llnl.util.lock`` allows locking to be enabled/disabled.""" import os import stat -import sys from typing import Optional, Tuple import spack.error @@ -38,7 +37,7 @@ def __init__( desc: str = "", enable: bool = True, ) -> None: - self._enable = sys.platform != "win32" and enable + self._enable = enable super().__init__( path, start=start, diff --git a/share/spack/qa/configuration/windows_locking_config.yaml b/share/spack/qa/configuration/windows_locking_config.yaml new file mode 100644 index 00000000000000..266c7c42df93b0 --- /dev/null +++ b/share/spack/qa/configuration/windows_locking_config.yaml @@ -0,0 +1,8 @@ +config: + locks: true + install_tree: + root: $spack\opt\spack + projections: + all: '${ARCHITECTURE}\${COMPILERNAME}-${COMPILERVER}\${PACKAGE}-${VERSION}-${HASH}' + build_stage: + - ~/.spack/stage