locks: only open lockfiles once instead of for every lock held (#24794)

This adds lockfile tracking to Spack's lock mechanism, so that we ensure that there
is only one open file descriptor per inode.

The `fcntl` locks that Spack uses are associated with an inode and a process.
This is convenient, because if a process exits, it releases its locks.
Unfortunately, this also means that if you close a file, *all* locks associated
with that file's inode are released, regardless of whether the process has any
other open file descriptors on it.

Because of this, we need to track open lock files so that we only close them when
a process no longer needs them.  We do this by tracking each lockfile by its
inode and process id.  This has several nice properties:

1. Tracking by pid ensures that, if we fork, we don't inadvertently track the parent
   process's lockfiles. `fcntl` locks are not inherited across forks, so we'll
   just track new lockfiles in the child.
2. Tracking by inode ensures that referencs are counted per inode, and that we don't
   inadvertently close a file whose inode still has open locks.
3. Tracking by both pid and inode ensures that we only open lockfiles the minimum
   number of times necessary for the locks we have.

Note: as mentioned elsewhere, these locks aren't thread safe -- they're designed to
work in Python and assume the GIL.

Tasks:
- [x] Introduce an `OpenFileTracker` class to track open file descriptors by inode.
- [x] Reference-count open file descriptors and only close them if they're no longer
      needed (this avoids inadvertently releasing locks that should not be released).

1 files changed, 128 insertions, 20 deletions

diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py
index 0c0cc7f634..c1544ab8ab 100644
--- a/lib/spack/llnl/util/lock.py
+++ b/lib/spack/llnl/util/lock.py
@@ -9,6 +9,7 @@ import os
 import socket
 import time
 from datetime import datetime
+from typing import Dict, Tuple  # novm
 
 import llnl.util.tty as tty
 
@@ -36,6 +37,126 @@ lock_type = {fcntl.LOCK_SH: 'read', fcntl.LOCK_EX: 'write'}
 true_fn = lambda: True
 
 
+class OpenFile(object):
+    """Record for keeping track of open lockfiles (with reference counting).
+
+    There's really only one ``OpenFile`` per inode, per process, but we record the
+    filehandle here as it's the thing we end up using in python code.  You can get
+    the file descriptor from the file handle if needed -- or we could make this track
+    file descriptors as well in the future.
+    """
+    def __init__(self, fh):
+        self.fh = fh
+        self.refs = 0
+
+
+class OpenFileTracker(object):
+    """Track open lockfiles, to minimize number of open file descriptors.
+
+    The ``fcntl`` locks that Spack uses are associated with an inode and a process.
+    This is convenient, because if a process exits, it releases its locks.
+    Unfortunately, this also means that if you close a file, *all* locks associated
+    with that file's inode are released, regardless of whether the process has any
+    other open file descriptors on it.
+
+    Because of this, we need to track open lock files so that we only close them when
+    a process no longer needs them.  We do this by tracking each lockfile by its
+    inode and process id.  This has several nice properties:
+
+    1. Tracking by pid ensures that, if we fork, we don't inadvertently track the parent
+       process's lockfiles. ``fcntl`` locks are not inherited across forks, so we'll
+       just track new lockfiles in the child.
+    2. Tracking by inode ensures that referencs are counted per inode, and that we don't
+       inadvertently close a file whose inode still has open locks.
+    3. Tracking by both pid and inode ensures that we only open lockfiles the minimum
+       number of times necessary for the locks we have.
+
+    Note: as mentioned elsewhere, these locks aren't thread safe -- they're designed to
+    work in Python and assume the GIL.
+    """
+
+    def __init__(self):
+        """Create a new ``OpenFileTracker``."""
+        self._descriptors = {}  # type: Dict[Tuple[int, int], OpenFile]
+
+    def get_fh(self, path):
+        """Get a filehandle for a lockfile.
+
+        This routine will open writable files for read/write even if you're asking
+        for a shared (read-only) lock. This is so that we can upgrade to an exclusive
+        (write) lock later if requested.
+
+        Arguments:
+          path (str): path to lock file we want a filehandle for
+        """
+        # Open writable files as 'r+' so we can upgrade to write later
+        os_mode, fh_mode = (os.O_RDWR | os.O_CREAT), 'r+'
+
+        pid = os.getpid()
+        open_file = None  # OpenFile object, if there is one
+        stat = None       # stat result for the lockfile, if it exists
+
+        try:
+            # see whether we've seen this inode/pid before
+            stat = os.stat(path)
+            key = (stat.st_ino, pid)
+            open_file = self._descriptors.get(key)
+
+        except OSError as e:
+            if e.errno != errno.ENOENT:  # only handle file not found
+                raise
+
+            # path does not exist -- fail if we won't be able to create it
+            parent = os.path.dirname(path) or '.'
+            if not os.access(parent, os.W_OK):
+                raise CantCreateLockError(path)
+
+        # if there was no already open file, we'll need to open one
+        if not open_file:
+            if stat and not os.access(path, os.W_OK):
+                # we know path exists but not if it's writable. If it's read-only,
+                # only open the file for reading (and fail if we're trying to get
+                # an exclusive (write) lock on it)
+                os_mode, fh_mode = os.O_RDONLY, 'r'
+
+            fd = os.open(path, os_mode)
+            fh = os.fdopen(fd, fh_mode)
+            open_file = OpenFile(fh)
+
+            # if we just created the file, we'll need to get its inode here
+            if not stat:
+                inode = os.fstat(fd).st_ino
+                key = (inode, pid)
+
+            self._descriptors[key] = open_file
+
+        open_file.refs += 1
+        return open_file.fh
+
+    def release_fh(self, path):
+        """Release a filehandle, only closing it if there are no more references."""
+        try:
+            inode = os.stat(path).st_ino
+        except OSError as e:
+            if e.errno != errno.ENOENT:  # only handle file not found
+                raise
+            inode = None  # this will not be in self._descriptors
+
+        key = (inode, os.getpid())
+        open_file = self._descriptors.get(key)
+        assert open_file, "Attempted to close non-existing lock path: %s" % path
+
+        open_file.refs -= 1
+        if not open_file.refs:
+            del self._descriptors[key]
+            open_file.fh.close()
+
+
+#: Open file descriptors for locks in this process. Used to prevent one process
+#: from opening the sam file many times for different byte range locks
+file_tracker = OpenFileTracker()
+
+
 def _attempts_str(wait_time, nattempts):
     # Don't print anything if we succeeded on the first try
     if nattempts <= 1:
@@ -56,7 +177,8 @@ class Lock(object):
     Note that this is for managing contention over resources *between*
     processes and not for managing contention between threads in a process: the
     functions of this object are not thread-safe. A process also must not
-    maintain multiple locks on the same file.
+    maintain multiple locks on the same file (or, more specifically, on
+    overlapping byte ranges in the same file).
     """
 
     def __init__(self, path, start=0, length=0, default_timeout=None,
@@ -161,25 +283,10 @@ class Lock(object):
 
         # Create file and parent directories if they don't exist.
         if self._file is None:
-            parent = self._ensure_parent_directory()
+            self._ensure_parent_directory()
+            self._file = file_tracker.get_fh(self.path)
 
-            # Open writable files as 'r+' so we can upgrade to write later
-            os_mode, fd_mode = (os.O_RDWR | os.O_CREAT), 'r+'
-            if os.path.exists(self.path):
-                if not os.access(self.path, os.W_OK):
-                    if op == fcntl.LOCK_SH:
-                        # can still lock read-only files if we open 'r'
-                        os_mode, fd_mode = os.O_RDONLY, 'r'
-                    else:
-                        raise LockROFileError(self.path)
-
-            elif not os.access(parent, os.W_OK):
-                raise CantCreateLockError(self.path)
-
-            fd = os.open(self.path, os_mode)
-            self._file = os.fdopen(fd, fd_mode)
-
-        elif op == fcntl.LOCK_EX and self._file.mode == 'r':
+        if op == fcntl.LOCK_EX and self._file.mode == 'r':
             # Attempt to upgrade to write lock w/a read-only file.
             # If the file were writable, we'd have opened it 'r+'
             raise LockROFileError(self.path)
@@ -292,7 +399,8 @@ class Lock(object):
         """
         fcntl.lockf(self._file, fcntl.LOCK_UN,
                     self._length, self._start, os.SEEK_SET)
-        self._file.close()
+
+        file_tracker.release_fh(self.path)
         self._file = None
         self._reads = 0
         self._writes = 0