tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix non-atomic set of transaction owner

Browse Source 
Summary: Remove static unlocked variable

Reviewers: buda

Reviewed By: buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1365
This commit is contained in:
Dominik Gleich 2018-04-17 15:15:22 +02:00
parent c9d2ad845c
commit 3ecf839198
3 changed files with 14 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/storage/locking/record_lock.cpp
View File

@ -45,9 +45,13 @@ std::experimental::optional<tx::transaction_id_t> FindOldestTxInLockCycle(
} // namespace } // namespace
bool RecordLock::TryLock(tx::transaction_id_t tx_id) {
tx::transaction_id_t unlocked{0};
return owner_.compare_exchange_strong(unlocked, tx_id);
}
LockStatus RecordLock::Lock(const tx::Transaction &tx, tx::Engine &engine) { LockStatus RecordLock::Lock(const tx::Transaction &tx, tx::Engine &engine) {
if (lock_.try_lock()) { if (TryLock(tx.id_)) {
owner_ = tx.id_;
return LockStatus::Acquired; return LockStatus::Acquired;
} }
@ -59,10 +63,8 @@ LockStatus RecordLock::Lock(const tx::Transaction &tx, tx::Engine &engine) {
// might be active for a dead transaction. By asking the transaction engine // might be active for a dead transaction. By asking the transaction engine
// for transaction info, we'll make the worker refresh it's knowledge about // for transaction info, we'll make the worker refresh it's knowledge about
// live transactions and release obsolete locks. // live transactions and release obsolete locks.
auto info = engine.Info(owner); if (owner == 0 || !engine.Info(owner).is_active()) {
if (!info.is_active()) { if (TryLock(tx.id_)) {
if (lock_.try_lock()) {
owner_ = tx.id_;
return LockStatus::Acquired; return LockStatus::Acquired;
} }
} }
@ -103,8 +105,7 @@ LockStatus RecordLock::Lock(const tx::Transaction &tx, tx::Engine &engine) {
throw LockTimeoutException( throw LockTimeoutException(
"Transaction was aborted since it was oldest in a lock cycle"); "Transaction was aborted since it was oldest in a lock cycle");
} }
if (lock_.try_lock()) { if (TryLock(tx.id_)) {
owner_ = tx.id_;
return LockStatus::Acquired; return LockStatus::Acquired;
} }
if (owner != owner_) { if (owner != owner_) {
@ -125,6 +126,6 @@ LockStatus RecordLock::Lock(const tx::Transaction &tx, tx::Engine &engine) {
"Transaction locked for more than {} seconds", kTimeout.count())); "Transaction locked for more than {} seconds", kTimeout.count()));
} }
void RecordLock::Unlock() { lock_.unlock(); } void RecordLock::Unlock() { owner_ = 0; }
constexpr std::chrono::duration<double> RecordLock::kTimeout; constexpr std::chrono::duration<double> RecordLock::kTimeout;

9
src/storage/locking/record_lock.hpp
View File

@ -12,7 +12,7 @@
namespace tx { namespace tx {
class Engine; class Engine;
class Transaction; class Transaction;
}; }; // namespace tx
class RecordLock { class RecordLock {
public: public:
@ -21,14 +21,11 @@ class RecordLock {
void Unlock(); void Unlock();
private: private:
bool TryLock(tx::transaction_id_t tx_id);
// Arbitrary choosen constant, postgresql uses 1 second so do we. // Arbitrary choosen constant, postgresql uses 1 second so do we.
constexpr static std::chrono::duration<double> kTimeout{ constexpr static std::chrono::duration<double> kTimeout{
std::chrono::seconds(1)}; std::chrono::seconds(1)};
// TODO: Because of the current architecture it is somewhat OK to use SpinLock
// here. Once we reimplement worker architecture to execute some other
// transaction in this thread while other is waiting for a lock this will had
// to change to something else.
SpinLock lock_;
std::atomic<tx::transaction_id_t> owner_{0}; std::atomic<tx::transaction_id_t> owner_{0};
}; };