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
main
Yanqin Jin 2 years ago committed by Facebook GitHub Bot
parent 08a63ad10b
commit 900f79126d
  1. 12
      utilities/transactions/lock/point/point_lock_manager.cc
  2. 7
      utilities/transactions/lock/point/point_lock_manager.h

@ -247,14 +247,14 @@ Status PointLockManager::TryLock(PessimisticTransaction* txn,
int64_t timeout = txn->GetLockTimeout(); int64_t timeout = txn->GetLockTimeout();
return AcquireWithTimeout(txn, lock_map, stripe, column_family_id, key, env, return AcquireWithTimeout(txn, lock_map, stripe, column_family_id, key, env,
timeout, std::move(lock_info)); timeout, lock_info);
} }
// Helper function for TryLock(). // Helper function for TryLock().
Status PointLockManager::AcquireWithTimeout( Status PointLockManager::AcquireWithTimeout(
PessimisticTransaction* txn, LockMap* lock_map, LockMapStripe* stripe, PessimisticTransaction* txn, LockMap* lock_map, LockMapStripe* stripe,
ColumnFamilyId column_family_id, const std::string& key, Env* env, 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; Status result;
uint64_t end_time = 0; uint64_t end_time = 0;
@ -278,7 +278,7 @@ Status PointLockManager::AcquireWithTimeout(
// Acquire lock if we are able to // Acquire lock if we are able to
uint64_t expire_time_hint = 0; uint64_t expire_time_hint = 0;
autovector<TransactionID> wait_ids; 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); &expire_time_hint, &wait_ids);
if (!result.ok() && timeout != 0) { if (!result.ok() && timeout != 0) {
@ -341,7 +341,7 @@ Status PointLockManager::AcquireWithTimeout(
} }
if (result.ok() || result.IsTimedOut()) { 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); &expire_time_hint, &wait_ids);
} }
} while (!result.ok() && !timed_out); } while (!result.ok() && !timed_out);
@ -475,7 +475,7 @@ bool PointLockManager::IncrementWaiters(
// REQUIRED: Stripe mutex must be held. // REQUIRED: Stripe mutex must be held.
Status PointLockManager::AcquireLocked(LockMap* lock_map, LockMapStripe* stripe, Status PointLockManager::AcquireLocked(LockMap* lock_map, LockMapStripe* stripe,
const std::string& key, Env* env, const std::string& key, Env* env,
LockInfo&& txn_lock_info, const LockInfo& txn_lock_info,
uint64_t* expire_time, uint64_t* expire_time,
autovector<TransactionID>* txn_ids) { autovector<TransactionID>* txn_ids) {
assert(txn_lock_info.txn_ids.size() == 1); 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); result = Status::Busy(Status::SubCode::kLockLimit);
} else { } else {
// acquire lock // 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 // Maintain lock count if there is a limit on the number of locks
if (max_num_locks_) { if (max_num_locks_) {

@ -200,11 +200,12 @@ class PointLockManager : public LockManager {
Status AcquireWithTimeout(PessimisticTransaction* txn, LockMap* lock_map, Status AcquireWithTimeout(PessimisticTransaction* txn, LockMap* lock_map,
LockMapStripe* stripe, uint32_t column_family_id, LockMapStripe* stripe, uint32_t column_family_id,
const std::string& key, Env* env, int64_t timeout, const std::string& key, Env* env, int64_t timeout,
LockInfo&& lock_info); const LockInfo& lock_info);
Status AcquireLocked(LockMap* lock_map, LockMapStripe* stripe, Status AcquireLocked(LockMap* lock_map, LockMapStripe* stripe,
const std::string& key, Env* env, LockInfo&& lock_info, const std::string& key, Env* env,
uint64_t* wait_time, autovector<TransactionID>* txn_ids); const LockInfo& lock_info, uint64_t* wait_time,
autovector<TransactionID>* txn_ids);
void UnLockKey(PessimisticTransaction* txn, const std::string& key, void UnLockKey(PessimisticTransaction* txn, const std::string& key,
LockMapStripe* stripe, LockMap* lock_map, Env* env); LockMapStripe* stripe, LockMap* lock_map, Env* env);

Loading…
Cancel
Save