Fix a bug and add more tests for nested locks.

tsdb2 · Dec 8, 2024 · 9e86516 · 9e86516
1 parent 88c07bd
commit 9e86516
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 7 deletions.
diff --git a/io/advisory_file_lock.cc b/io/advisory_file_lock.cc
@@ -30,10 +30,9 @@ ExclusiveFileLock::InternalLock::LockSet ExclusiveFileLock::InternalLock::locks_
 
 absl::StatusOr<tsdb2::common::reffed_ptr<ExclusiveFileLock::InternalLock>>
 ExclusiveFileLock::InternalLock::Acquire(FD const &fd, off_t const start, off_t const length) {
-  DEFINE_VAR_OR_RETURN(fd2, fd.Clone());
   struct stat statbuf;
   std::memset(&statbuf, 0, sizeof(struct stat));
-  if (::fstat(*fd2, &statbuf) < 0) {
+  if (::fstat(*fd, &statbuf) < 0) {
     return absl::ErrnoToStatus(errno, "fstat");
   }
   absl::MutexLock lock{&mutex_};
@@ -45,13 +44,14 @@ ExclusiveFileLock::InternalLock::Acquire(FD const &fd, off_t const start, off_t
   if (it != locks_.end()) {
     return tsdb2::common::WrapReffed(const_cast<InternalLock *>(&*it));
   }
+  DEFINE_VAR_OR_RETURN(fd2, fd.Clone());
   struct flock args;
   std::memset(&args, 0, sizeof(struct flock));
   args.l_type = F_WRLCK;
   args.l_whence = SEEK_SET;
   args.l_start = start;
   args.l_len = length;
-  if (::fcntl(fd.get(), F_SETLKW, &args) < 0) {
+  if (::fcntl(fd2.get(), F_SETLKW, &args) < 0) {
     return absl::ErrnoToStatus(errno, "fcntl(F_SETLKW, F_WRLCK)");
   }
   bool inserted;

diff --git a/io/advisory_file_lock_test.cc b/io/advisory_file_lock_test.cc
@@ -31,6 +31,15 @@ TEST(AdvisoryFileLockTest, Acquire) {
   EXPECT_OK(ExclusiveFileLock::Acquire(file.fd()));
 }
 
+TEST(AdvisoryFileLockTest, NestedAcquire) {
+  auto const status_or_file = TestTempFile::Create(kTestFileName);
+  ASSERT_OK(status_or_file);
+  auto const& file = status_or_file.value();
+  auto const status_or_lock1 = ExclusiveFileLock::Acquire(file.fd());
+  ASSERT_OK(status_or_lock1);
+  EXPECT_OK(ExclusiveFileLock::Acquire(file.fd()));
+}
+
 TEST(AdvisoryFileLockTest, FullRange) {
   auto const status_or_file = TestTempFile::Create(kTestFileName);
   ASSERT_OK(status_or_file);
@@ -53,13 +62,84 @@ TEST(AdvisoryFileLockTest, FullRange) {
     ASSERT_OK(SigUsr1::Notify(pid));
     ASSERT_GE(::waitpid(pid, nullptr, 0), 0) << absl::ErrnoToStatus(errno, "waitpid");
   } else {  // child
-    auto const status_or_lock = ExclusiveFileLock::Acquire(file.fd());
-    EXPECT_OK(status_or_lock);
-    ASSERT_OK(sigusr1.Notify());
+    {
+      auto const status_or_lock = ExclusiveFileLock::Acquire(file.fd());
+      EXPECT_OK(status_or_lock);
+      ASSERT_OK(sigusr1.Notify());
+      ASSERT_OK(sigusr1.WaitForNotification());
+    }
+    ::_exit(0);
+  }
+}
+
+TEST(AdvisoryFileLockTest, NestedLocks) {
+  auto const status_or_file = TestTempFile::Create(kTestFileName);
+  ASSERT_OK(status_or_file);
+  auto const& file = status_or_file.value();
+  SigUsr1 sigusr1;
+  auto const pid = ::fork();
+  ASSERT_GE(pid, 0) << absl::ErrnoToStatus(errno, "fork");
+  if (pid) {  // parent
     ASSERT_OK(sigusr1.WaitForNotification());
+    LockInfo fl;
+    std::memset(&fl, 0, sizeof(LockInfo));
+    fl.l_type = F_WRLCK;
+    fl.l_whence = SEEK_SET;
+    fl.l_start = 0;
+    fl.l_len = 1;
+    ASSERT_LE(::fcntl(file.fd().get(), F_GETLK, &fl), 0) << absl::ErrnoToStatus(errno, "fnctl");
+    EXPECT_THAT(fl, AllOf(Field(&LockInfo::l_type, F_WRLCK), Field(&LockInfo::l_whence, SEEK_SET),
+                          Field(&LockInfo::l_start, 0), Field(&LockInfo::l_len, 0),
+                          Field(&LockInfo::l_pid, pid)));
+    ASSERT_OK(SigUsr1::Notify(pid));
+    ASSERT_GE(::waitpid(pid, nullptr, 0), 0) << absl::ErrnoToStatus(errno, "waitpid");
+  } else {  // child
+    {
+      auto const status_or_lock1 = ExclusiveFileLock::Acquire(file.fd());
+      ASSERT_OK(status_or_lock1);
+      auto const status_or_lock2 = ExclusiveFileLock::Acquire(file.fd());
+      EXPECT_OK(status_or_lock2);
+      ASSERT_OK(sigusr1.Notify());
+      ASSERT_OK(sigusr1.WaitForNotification());
+    }
+    ::_exit(0);
   }
 }
 
-// TODO
+TEST(AdvisoryFileLockTest, InnerLockReleased) {
+  auto const status_or_file = TestTempFile::Create(kTestFileName);
+  ASSERT_OK(status_or_file);
+  auto const& file = status_or_file.value();
+  SigUsr1 sigusr1;
+  auto const pid = ::fork();
+  ASSERT_GE(pid, 0) << absl::ErrnoToStatus(errno, "fork");
+  if (pid) {  // parent
+    ASSERT_OK(sigusr1.WaitForNotification());
+    LockInfo fl;
+    std::memset(&fl, 0, sizeof(LockInfo));
+    fl.l_type = F_WRLCK;
+    fl.l_whence = SEEK_SET;
+    fl.l_start = 0;
+    fl.l_len = 1;
+    ASSERT_LE(::fcntl(file.fd().get(), F_GETLK, &fl), 0) << absl::ErrnoToStatus(errno, "fnctl");
+    EXPECT_THAT(fl, AllOf(Field(&LockInfo::l_type, F_WRLCK), Field(&LockInfo::l_whence, SEEK_SET),
+                          Field(&LockInfo::l_start, 0), Field(&LockInfo::l_len, 0),
+                          Field(&LockInfo::l_pid, pid)));
+    ASSERT_OK(SigUsr1::Notify(pid));
+    ASSERT_GE(::waitpid(pid, nullptr, 0), 0) << absl::ErrnoToStatus(errno, "waitpid");
+  } else {  // child
+    {
+      auto const status_or_lock1 = ExclusiveFileLock::Acquire(file.fd());
+      ASSERT_OK(status_or_lock1);
+      {
+        auto const status_or_lock2 = ExclusiveFileLock::Acquire(file.fd());
+        EXPECT_OK(status_or_lock2);
+      }
+      ASSERT_OK(sigusr1.Notify());
+      ASSERT_OK(sigusr1.WaitForNotification());
+    }
+    ::_exit(0);
+  }
+}
 
 }  // namespace