* Make lock removal safe by signalling to blocked processes that the

  lock they are waiting on has become stale (we do this by writing a
  meaningless token to the unlinked file).

4 files changed, 55 insertions, 24 deletions

diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc
index 79ccf7d663f6..a92b2225a51c 100644
--- a/src/libstore/pathlocks.cc
+++ b/src/libstore/pathlocks.cc
@@ -61,7 +61,7 @@ PathLocks::PathLocks(const PathSet & paths)
 void PathLocks::lockPaths(const PathSet & _paths)
 {
     /* May be called only once! */
-    assert(this->paths.empty());
+    assert(fds.empty());
     
     /* Note that `fds' is built incrementally so that the destructor
        will only release those locks that we have already acquired. */
@@ -83,20 +83,38 @@ void PathLocks::lockPaths(const PathSet & _paths)
             debug(format("already holding lock on `%1%'") % lockPath);
             continue;
         }
-        
-        /* Open/create the lock file. */
-        int fd = open(lockPath.c_str(), O_WRONLY | O_CREAT, 0666);
-        if (fd == -1)
-            throw SysError(format("opening lock file `%1%'") % lockPath);
-
-        fds.push_back(fd);
-        this->paths.push_back(lockPath);
-
-        /* Acquire an exclusive lock. */
-        lockFile(fd, ltWrite, true);
 
-        debug(format("lock acquired on `%1%'") % lockPath);
+        AutoCloseFD fd;
+        
+        while (1) {
+        
+            /* Open/create the lock file. */
+            fd = open(lockPath.c_str(), O_WRONLY | O_CREAT, 0666);
+            if (fd == -1)
+                throw SysError(format("opening lock file `%1%'") % lockPath);
+
+            /* Acquire an exclusive lock. */
+            lockFile(fd, ltWrite, true);
+
+            debug(format("lock acquired on `%1%'") % lockPath);
+
+            /* Check that the lock file hasn't become stale (i.e.,
+               hasn't been unlinked). */
+            struct stat st;
+            if (fstat(fd, &st) == -1)
+                throw SysError(format("statting lock file `%1%'") % lockPath);
+            if (st.st_size != 0)
+                /* This lock file has been unlinked, so we're holding
+                   a lock on a deleted file.  This means that other
+                   processes may create and acquire a lock on
+                   `lockPath', and proceed.  So we must retry. */
+                debug(format("open lock file `%1%' has become stale") % lockPath);
+            else
+                break;
+        }
 
+        /* Use borrow so that the descriptor isn't closed. */
+        fds.push_back(FDPair(fd.borrow(), lockPath));
         lockedPaths.insert(lockPath);
     }
 }
@@ -104,19 +122,22 @@ void PathLocks::lockPaths(const PathSet & _paths)
 
 PathLocks::~PathLocks()
 {
-    for (list<int>::iterator i = fds.begin(); i != fds.end(); i++)
-        if (close(*i) != 0) throw SysError("closing fd");
-
-    for (Paths::iterator i = paths.begin(); i != paths.end(); i++) {
-        checkInterrupt();
+    for (list<FDPair>::iterator i = fds.begin(); i != fds.end(); i++) {
         if (deletePaths) {
-            /* This is not safe in general! */
-            unlink(i->c_str());
+            /* Write a (meaningless) token to the file to indicate to
+               other processes waiting on this lock that the lock is
+               stale (deleted). */
+            if (write(i->first, "d", 1) == 1) {
+                unlink(i->second.c_str());
+            }
             /* Note that the result of unlink() is ignored; removing
                the lock file is an optimisation, not a necessity. */
         }
-        lockedPaths.erase(*i);
-        debug(format("lock released on `%1%'") % *i);
+        lockedPaths.erase(i->second);
+        if (close(i->first) == -1)
+            printMsg(lvlError,
+                format("error (ignored): cannot close lock file on `%1%'") % i->second);
+        debug(format("lock released on `%1%'") % i->second);
     }
 }
 
diff --git a/src/libstore/pathlocks.hh b/src/libstore/pathlocks.hh
index 433438906934..42ebe58df691 100644
--- a/src/libstore/pathlocks.hh
+++ b/src/libstore/pathlocks.hh
@@ -12,8 +12,8 @@ bool lockFile(int fd, LockType lockType, bool wait);
 class PathLocks 
 {
 private:
-    list<int> fds;
-    Paths paths;
+    typedef pair<int, Path> FDPair;
+    list<FDPair> fds;
     bool deletePaths;
 
 public:
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index e77009321d8d..0af6ee149bae 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -415,6 +415,15 @@ bool AutoCloseFD::isOpen()
 }
 
 
+/* Pass responsibility for closing this fd to the caller. */
+int AutoCloseFD::borrow()
+{
+    int oldFD = fd;
+    fd = -1;
+    return oldFD;
+}
+
+
 void Pipe::create()
 {
     int fds[2];
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index a79c07305df8..d947c34252a7 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -184,6 +184,7 @@ public:
     operator int();
     void close();
     bool isOpen();
+    int borrow();
 };