Fix accessor locking in storage v2

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

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

src/storage/v2/storage.cpp

@@ -303,12 +303,14 @@ Storage::~Storage() {
   }
 }
 
-Storage::Accessor::Accessor(Storage *storage, uint64_t transaction_id,
-                            uint64_t start_timestamp)
+Storage::Accessor::Accessor(Storage *storage)
     : storage_(storage),
-      transaction_(transaction_id, start_timestamp),
-      is_transaction_active_(true),
-      storage_guard_(storage_->main_lock_) {}
+      // The lock must be acquired before creating the transaction object to
+      // prevent freshly created transactions from dangling in an active state
+      // during exclusive operations.
+      storage_guard_(storage_->main_lock_),
+      transaction_(storage->CreateTransaction()),
+      is_transaction_active_(true) {}
 
 Storage::Accessor::Accessor(Accessor &&other) noexcept
     : storage_(other.storage_),
@@ -845,20 +847,6 @@ void Storage::Accessor::Abort() {
   }
 }
 
-Storage::Accessor Storage::Access() {
-  // We acquire the transaction engine lock here because we access (and
-  // modify) the transaction engine variables (`transaction_id` and
-  // `timestamp`) below.
-  uint64_t transaction_id;
-  uint64_t start_timestamp;
-  {
-    std::lock_guard<utils::SpinLock> guard(engine_lock_);
-    transaction_id = transaction_id_++;
-    start_timestamp = timestamp_++;
-  }
-  return Accessor{this, transaction_id, start_timestamp};
-}
-
 const std::string &Storage::LabelToName(LabelId label) const {
   return name_id_mapper_.IdToName(label.AsUint());
 }
@@ -910,6 +898,20 @@ VerticesIterable Storage::Accessor::Vertices(
       label, property, lower_bound, upper_bound, view, &transaction_));
 }
 
+Transaction Storage::CreateTransaction() {
+  // We acquire the transaction engine lock here because we access (and
+  // modify) the transaction engine variables (`transaction_id` and
+  // `timestamp`) below.
+  uint64_t transaction_id;
+  uint64_t start_timestamp;
+  {
+    std::lock_guard<utils::SpinLock> guard(engine_lock_);
+    transaction_id = transaction_id_++;
+    start_timestamp = timestamp_++;
+  }
+  return {transaction_id, start_timestamp};
+}
+
 void Storage::CollectGarbage() {
   // Garbage collection must be performed in two phases. In the first phase,
   // deltas that won't be applied by any transaction anymore are unlinked from

src/storage/v2/storage.hpp

@@ -176,8 +176,7 @@ class Storage final {
    private:
     friend class Storage;
 
-    Accessor(Storage *storage, uint64_t transaction_id,
-             uint64_t start_timestamp);
+    explicit Accessor(Storage *storage);
 
    public:
     Accessor(const Accessor &) = delete;
@@ -296,13 +295,12 @@ class Storage final {
 
    private:
     Storage *storage_;
+    std::shared_lock<utils::RWLock> storage_guard_;
     Transaction transaction_;
     bool is_transaction_active_;
-
-    std::shared_lock<utils::RWLock> storage_guard_;
   };
 
-  Accessor Access();
+  Accessor Access() { return Accessor{this}; }
 
   const std::string &LabelToName(LabelId label) const;
   const std::string &PropertyToName(PropertyId property) const;
@@ -364,6 +362,8 @@ class Storage final {
   }
 
  private:
+  Transaction CreateTransaction();
+
   /// @throw std::system_error
   /// @throw std::bad_alloc
   void CollectGarbage();