Fix bug in lock store

Reviewers: buda, mferencevic

Reviewed By: mferencevic

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D594

src/storage/locking/record_lock.cpp

@@ -1,17 +1,17 @@
 #include "storage/locking/record_lock.hpp"
 
-void RecordLock::lock() { mutex.lock(&timeout); }
+LockStatus RecordLock::Lock(tx::transaction_id_t id) {
+  if (mutex_.try_lock()) {
+    owner_ = id;
+    return LockStatus::Acquired;
+  }
 
-LockStatus RecordLock::lock(tx::transaction_id_t id) {
+  if (owner_ == id) return LockStatus::AlreadyHeld;
-  if (mutex.try_lock()) return owner = id, LockStatus::Acquired;
 
-  if (owner == id) return LockStatus::AlreadyHeld;
+  mutex_.lock(&kTimeout);
-
+  return LockStatus::Acquired;
-  return mutex.lock(&timeout), LockStatus::Acquired;
 }
 
-void RecordLock::unlock() {
+void RecordLock::Unlock() { mutex_.unlock(); }
-  mutex.unlock();
-}
 
-constexpr struct timespec RecordLock::timeout;
+constexpr struct timespec RecordLock::kTimeout;

src/storage/locking/record_lock.hpp

@@ -1,19 +1,18 @@
 #pragma once
 
-#include "transactions/type.hpp"
 #include "storage/locking/lock_status.hpp"
 #include "threading/sync/futex.hpp"
+#include "transactions/type.hpp"
 
 class RecordLock {
   // TODO arbitrary constant, reconsider
-  static constexpr struct timespec timeout { 2, 0 };
+  static constexpr struct timespec kTimeout { 2, 0 };
 
  public:
-  LockStatus lock(tx::transaction_id_t id);
+  LockStatus Lock(tx::transaction_id_t id);
-  void lock();
+  void Unlock();
-  void unlock();
 
  private:
-  Futex mutex;
+  Futex mutex_;
-  tx::transaction_id_t owner;
+  tx::transaction_id_t owner_;
 };

src/transactions/lock_store.hpp

@@ -11,42 +11,53 @@ template <class T>
 class LockStore {
   class LockHolder {
    public:
-    LockHolder() noexcept = default;
+    LockHolder() = default;
 
     template <class... Args>
-    LockHolder(T *lock, Args &&... args) : lock(lock) {
+    LockHolder(T *lock, Args &&... args) : lock_(lock) {
       debug_assert(lock != nullptr, "Lock is nullptr.");
-      auto status = lock->lock(std::forward<Args>(args)...);
+      auto status = lock_->Lock(std::forward<Args>(args)...);
 
-      if (status != LockStatus::Acquired) lock = nullptr;
+      if (status != LockStatus::Acquired) {
+        lock_ = nullptr;
+      }
     }
 
     LockHolder(const LockHolder &) = delete;
-    LockHolder(LockHolder &&other) noexcept : lock(other.lock) {
+    LockHolder &operator=(const LockHolder &) = delete;
-      other.lock = nullptr;
+
+    LockHolder(LockHolder &&other) : lock_(other.lock_) {
+      other.lock_ = nullptr;
+    }
+
+    LockHolder &operator=(LockHolder &&other) {
+      if (this == &other) return *this;
+      lock_ = other.lock_;
+      other.lock_ = nullptr;
     }
 
     ~LockHolder() {
-      if (lock != nullptr) lock->unlock();
+      if (lock_ != nullptr) {
+        lock_->Unlock();
+      }
     }
 
-    bool active() const noexcept { return lock != nullptr; }
+    bool active() const { return lock_ != nullptr; }
 
    private:
-    T *lock{nullptr};
+    T *lock_{nullptr};
   };
 
  public:
   template <class... Args>
-  void take(T *lock, Args &&... args) {
+  void Take(T *lock, Args &&... args) {
-    locks.emplace_back(LockHolder(lock, std::forward<Args>(args)...));
+    locks_.emplace_back(LockHolder(lock, std::forward<Args>(args)...));
-    if (!locks.back().active()) {
+    if (!locks_.back().active()) {
-      locks.pop_back();
+      locks_.pop_back();
-      return;
     }
   }
 
  private:
-  std::vector<LockHolder> locks;
+  std::vector<LockHolder> locks_;
-};
 };
+}

src/transactions/transaction.cpp

@@ -8,7 +8,7 @@ Transaction::Transaction(transaction_id_t id, const Snapshot &snapshot,
                          Engine &engine)
     : id_(id), engine_(engine), snapshot_(snapshot) {}
 
-void Transaction::TakeLock(RecordLock &lock) { locks_.take(&lock, id_); }
+void Transaction::TakeLock(RecordLock &lock) { locks_.Take(&lock, id_); }
 
 void Transaction::Commit() { engine_.Commit(*this); }