From 900f79126dc801199006562550aadd998c5217a8 Mon Sep 17 00:00:00 2001
From: Yanqin Jin <yanqin@fb.com>
Date: Fri, 28 Oct 2022 14:05:12 -0700
Subject: [PATCH] Pass `const LockInfo&` to AcquireLocked() and
 AcquireWithTimeout (#10874)

Summary:
The motivation and benefit of current behavior of passing `LockInfo&&` as argument to AcquireLocked() and AcquireWithTimeout() is not clear to me. Furthermore, in AcquireWithTimeout(), we access members of `LockInfo&&` after it is passed to AcquireLocked() as rvalue ref. In addition, we may call `AcquireLocked()` with `std::move(lock_info)` multiple times.

This leads to linter warning of use-after-move. If future implementation of AcquireLocked() does something like moving-construct a new `LockedInfo` using the passed-in `LockInfo&&`, then the caller cannot use it because `LockInfo` has a member of type `autovector`.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/10874

Test Plan: make check

Reviewed By: ltamasi

Differential Revision: D40704210

Pulled By: riversand963

fbshipit-source-id: 20091df65b4fc63b072bcec9809efc49955d6d35
---
 .../transactions/lock/point/point_lock_manager.cc    | 12 ++++++------
 .../transactions/lock/point/point_lock_manager.h     |  7 ++++---
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/utilities/transactions/lock/point/point_lock_manager.cc b/utilities/transactions/lock/point/point_lock_manager.cc
index c71a66027..b362a164d 100644
--- a/utilities/transactions/lock/point/point_lock_manager.cc
+++ b/utilities/transactions/lock/point/point_lock_manager.cc
@@ -247,14 +247,14 @@ Status PointLockManager::TryLock(PessimisticTransaction* txn,
   int64_t timeout = txn->GetLockTimeout();
 
   return AcquireWithTimeout(txn, lock_map, stripe, column_family_id, key, env,
-                            timeout, std::move(lock_info));
+                            timeout, lock_info);
 }
 
 // Helper function for TryLock().
 Status PointLockManager::AcquireWithTimeout(
     PessimisticTransaction* txn, LockMap* lock_map, LockMapStripe* stripe,
     ColumnFamilyId column_family_id, const std::string& key, Env* env,
-    int64_t timeout, LockInfo&& lock_info) {
+    int64_t timeout, const LockInfo& lock_info) {
   Status result;
   uint64_t end_time = 0;
 
@@ -278,7 +278,7 @@ Status PointLockManager::AcquireWithTimeout(
   // Acquire lock if we are able to
   uint64_t expire_time_hint = 0;
   autovector<TransactionID> wait_ids;
-  result = AcquireLocked(lock_map, stripe, key, env, std::move(lock_info),
+  result = AcquireLocked(lock_map, stripe, key, env, lock_info,
                          &expire_time_hint, &wait_ids);
 
   if (!result.ok() && timeout != 0) {
@@ -341,7 +341,7 @@ Status PointLockManager::AcquireWithTimeout(
       }
 
       if (result.ok() || result.IsTimedOut()) {
-        result = AcquireLocked(lock_map, stripe, key, env, std::move(lock_info),
+        result = AcquireLocked(lock_map, stripe, key, env, lock_info,
                                &expire_time_hint, &wait_ids);
       }
     } while (!result.ok() && !timed_out);
@@ -475,7 +475,7 @@ bool PointLockManager::IncrementWaiters(
 // REQUIRED:  Stripe mutex must be held.
 Status PointLockManager::AcquireLocked(LockMap* lock_map, LockMapStripe* stripe,
                                        const std::string& key, Env* env,
-                                       LockInfo&& txn_lock_info,
+                                       const LockInfo& txn_lock_info,
                                        uint64_t* expire_time,
                                        autovector<TransactionID>* txn_ids) {
   assert(txn_lock_info.txn_ids.size() == 1);
@@ -527,7 +527,7 @@ Status PointLockManager::AcquireLocked(LockMap* lock_map, LockMapStripe* stripe,
       result = Status::Busy(Status::SubCode::kLockLimit);
     } else {
       // acquire lock
-      stripe->keys.emplace(key, std::move(txn_lock_info));
+      stripe->keys.emplace(key, txn_lock_info);
 
       // Maintain lock count if there is a limit on the number of locks
       if (max_num_locks_) {
diff --git a/utilities/transactions/lock/point/point_lock_manager.h b/utilities/transactions/lock/point/point_lock_manager.h
index 5f01eaeb2..eeb34f3be 100644
--- a/utilities/transactions/lock/point/point_lock_manager.h
+++ b/utilities/transactions/lock/point/point_lock_manager.h
@@ -200,11 +200,12 @@ class PointLockManager : public LockManager {
   Status AcquireWithTimeout(PessimisticTransaction* txn, LockMap* lock_map,
                             LockMapStripe* stripe, uint32_t column_family_id,
                             const std::string& key, Env* env, int64_t timeout,
-                            LockInfo&& lock_info);
+                            const LockInfo& lock_info);
 
   Status AcquireLocked(LockMap* lock_map, LockMapStripe* stripe,
-                       const std::string& key, Env* env, LockInfo&& lock_info,
-                       uint64_t* wait_time, autovector<TransactionID>* txn_ids);
+                       const std::string& key, Env* env,
+                       const LockInfo& lock_info, uint64_t* wait_time,
+                       autovector<TransactionID>* txn_ids);
 
   void UnLockKey(PessimisticTransaction* txn, const std::string& key,
                  LockMapStripe* stripe, LockMap* lock_map, Env* env);