Fix for Issue:1780

util/env_posix.cc

@@ -76,27 +76,6 @@ static std::set<std::string> lockedFiles;
 static port::Mutex mutex_lockedFiles;
 
 static int LockOrUnlock(const std::string& fname, int fd, bool lock) {
-  mutex_lockedFiles.Lock();
-  if (lock) {
-    // If it already exists in the lockedFiles set, then it is already locked,
-    // and fail this lock attempt. Otherwise, insert it into lockedFiles.
-    // This check is needed because fcntl() does not detect lock conflict
-    // if the fcntl is issued by the same thread that earlier acquired
-    // this lock.
-    if (lockedFiles.insert(fname).second == false) {
-      mutex_lockedFiles.Unlock();
-      errno = ENOLCK;
-      return -1;
-    }
-  } else {
-    // If we are unlocking, then verify that we had locked it earlier,
-    // it should already exist in lockedFiles. Remove it from lockedFiles.
-    if (lockedFiles.erase(fname) != 1) {
-      mutex_lockedFiles.Unlock();
-      errno = ENOLCK;
-      return -1;
-    }
-  }
   errno = 0;
   struct flock f;
   memset(&f, 0, sizeof(f));
@@ -105,11 +84,7 @@ static int LockOrUnlock(const std::string& fname, int fd, bool lock) {
   f.l_start = 0;
   f.l_len = 0;        // Lock/unlock entire file
   int value = fcntl(fd, F_SETLK, &f);
-  if (value == -1 && lock) {
-    // if there is an error in locking, then remove the pathname from lockedfiles
-    lockedFiles.erase(fname);
-  }
-  mutex_lockedFiles.Unlock();
+
   return value;
 }
 
@@ -548,6 +523,23 @@ class PosixEnv : public Env {
   virtual Status LockFile(const std::string& fname, FileLock** lock) override {
     *lock = nullptr;
     Status result;
+
+    mutex_lockedFiles.Lock();
+    // If it already exists in the lockedFiles set, then it is already locked,
+    // and fail this lock attempt. Otherwise, insert it into lockedFiles.
+    // This check is needed because fcntl() does not detect lock conflict
+    // if the fcntl is issued by the same thread that earlier acquired
+    // this lock.
+    // We must do this check *before* opening the file:
+    // Otherwise, we will open a new file descriptor. Locks are associated with
+    // a process, not a file descriptor and when *any* file descriptor is closed,
+    // all locks the process holds for that *file* are released
+    if (lockedFiles.insert(fname).second == false) {
+      mutex_lockedFiles.Unlock();
+      errno = ENOLCK;
+      return IOError("lock " + fname, errno);
+    }
+
     int fd;
     {
       IOSTATS_TIMER_GUARD(open_nanos);
@@ -556,6 +548,8 @@ class PosixEnv : public Env {
     if (fd < 0) {
       result = IOError(fname, errno);
     } else if (LockOrUnlock(fname, fd, true) == -1) {
+      // if there is an error in locking, then remove the pathname from lockedfiles
+      lockedFiles.erase(fname);
       result = IOError("lock " + fname, errno);
       close(fd);
     } else {
@@ -565,17 +559,27 @@ class PosixEnv : public Env {
       my_lock->filename = fname;
       *lock = my_lock;
     }
+
+    mutex_lockedFiles.Unlock();
     return result;
   }
 
   virtual Status UnlockFile(FileLock* lock) override {
     PosixFileLock* my_lock = reinterpret_cast<PosixFileLock*>(lock);
     Status result;
-    if (LockOrUnlock(my_lock->filename, my_lock->fd_, false) == -1) {
+
+    mutex_lockedFiles.Lock();
+    // If we are unlocking, then verify that we had locked it earlier,
+    // it should already exist in lockedFiles. Remove it from lockedFiles.
+    if (lockedFiles.erase(my_lock->filename) != 1) {
+      errno = ENOLCK;
+      result = IOError("unlock", errno);
+    } else if (LockOrUnlock(my_lock->filename, my_lock->fd_, false) == -1) {
       result = IOError("unlock", errno);
     }
     close(my_lock->fd_);
     delete my_lock;
+    mutex_lockedFiles.Unlock();
     return result;
   }
 

util/filelock_test.cc

@@ -7,6 +7,7 @@
 #include "rocksdb/env.h"
 
 #include <vector>
+#include <fcntl.h>
 #include "util/coding.h"
 #include "util/testharness.h"
 
@@ -33,6 +34,78 @@ class LockTest : public testing::Test {
   Status UnlockFile(FileLock* db_lock) {
     return env_->UnlockFile(db_lock);
   }
+
+  bool AssertFileIsLocked(){
+    return CheckFileLock( /* lock_expected = */ true);
+  }
+
+  bool AssertFileIsNotLocked(){
+    return CheckFileLock( /* lock_expected = */ false);
+  }
+
+  bool CheckFileLock(bool lock_expected){
+    // We need to fork to check the fcntl lock as we need
+    // to open and close the file from a different process
+    // to avoid either releasing the lock on close, or not
+    // contending for it when requesting a lock.
+
+#ifdef OS_WIN
+
+    // WaitForSingleObject and GetExitCodeProcess can do what waitpid does.
+    // TODO - implement on Windows
+    return true;
+
+#else
+
+    pid_t pid = fork();
+    if ( 0 == pid ) {
+      // child process
+      int exit_val = EXIT_FAILURE;
+      int fd = open(file_.c_str(), O_RDWR | O_CREAT, 0644);
+      if (fd < 0) {
+        // could not open file, could not check if it was locked
+        fprintf( stderr, "Open on on file %s failed.\n",file_.c_str());
+        exit(exit_val);
+      }
+
+      struct flock f;
+      memset(&f, 0, sizeof(f));
+      f.l_type = (F_WRLCK);
+      f.l_whence = SEEK_SET;
+      f.l_start = 0;
+      f.l_len = 0; // Lock/unlock entire file
+      int value = fcntl(fd, F_SETLK, &f);
+      if( value == -1 ){
+        if( lock_expected ){
+          exit_val = EXIT_SUCCESS;
+        }
+      } else {
+        if( ! lock_expected ){
+          exit_val = EXIT_SUCCESS;
+        }
+      }
+      close(fd); // lock is released for child process
+      exit(exit_val);
+    } else if (pid > 0) {
+      // parent process
+      int status;
+      while (-1 == waitpid(pid, &status, 0));
+      if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+        // child process exited with non success status
+        return false;
+      } else {
+        return true;
+      }
+    } else {
+      fprintf( stderr, "Fork failed\n" );
+      return false;
+    }
+    return false;
+
+#endif
+
+  }
+
 };
 LockTest* LockTest::current_;
 
@@ -43,12 +116,21 @@ TEST_F(LockTest, LockBySameThread) {
   // acquire a lock on a file
   ASSERT_OK(LockFile(&lock1));
 
+  // check the file is locked
+  ASSERT_TRUE( AssertFileIsLocked() );
+
   // re-acquire the lock on the same file. This should fail.
   ASSERT_TRUE(LockFile(&lock2).IsIOError());
 
+  // check the file is locked
+  ASSERT_TRUE( AssertFileIsLocked() );
+
   // release the lock
   ASSERT_OK(UnlockFile(lock1));
 
+  // check the file is not locked
+  ASSERT_TRUE( AssertFileIsNotLocked() );
+
 }
 
 }  // namespace rocksdb