From 59682e618805701f9c249736514df6db457895f9 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 27 Jan 2005 12:19:25 +0000 Subject: [PATCH] * 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). --- src/libstore/pathlocks.cc | 59 ++++++++++++++++++++++++++------------- src/libstore/pathlocks.hh | 4 +-- src/libutil/util.cc | 9 ++++++ src/libutil/util.hh | 1 + 4 files changed, 52 insertions(+), 21 deletions(-) diff --git a/src/libstore/pathlocks.cc b/src/libstore/pathlocks.cc index 79ccf7d66..a92b2225a 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; } + + AutoCloseFD fd; - /* 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); + 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); - fds.push_back(fd); - this->paths.push_back(lockPath); + /* Acquire an exclusive lock. */ + lockFile(fd, ltWrite, true); - /* Acquire an exclusive lock. */ - lockFile(fd, ltWrite, true); + debug(format("lock acquired on `%1%'") % lockPath); - 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::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::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 433438906..42ebe58df 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 fds; - Paths paths; + typedef pair FDPair; + list fds; bool deletePaths; public: diff --git a/src/libutil/util.cc b/src/libutil/util.cc index e77009321..0af6ee149 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 a79c07305..d947c3425 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -184,6 +184,7 @@ public: operator int(); void close(); bool isOpen(); + int borrow(); };