fix(transaction): Add special barrier for blocking tx #2512

dranikpg · 2024-01-31T17:42:05Z

Currently we assume waking a transaction is exlusive and we ensured exclusivity with

if (wakeup_requested_.fetch_add(1, memory_order_relaxed) > 0) return

in NotifySuspended()

However we ensured no exclusivity with cancelling or timing out. I.e. it was totally possible for a transaction to time out and start preparing the cleanup hops, while another thread would call NotifySuspended(). What is more, we removed COORD_BLOCKED only after we called Expire, so we could be cancelled while we are expiring. This results in a lot of unsafe data parallel accesses.

Another problem here is also that once we do

wakeup_requested_.fetch_add(1, memory_order_relaxed)

we technically allow blocking_ec_ in the coordinator to pass. If it wasn't suspended and just reached the check (fast path), it will proceed without waiting for the foreign thread to actually finish modifying the values. Paired with no proper acquire/release semantics later on, this is a very unsafe inter-thread exchange

Instead I suggest to use SingleClaimBarrier

"Single claim - single modification" barrier. Multiple threads might try to claim it, only one
will succeed and will be allowed to modify the barrier until it releases it.

with proper expiration

Wait for barrier until time_point, indefinitely if time_point::max() was passed.
Expiration plays by the same rules as all other threads, it will try to claim the barrier or
wait for an ongoing modification to release.

It results in very simple guarantees:

If any actor (blocking controller, expiration, cancellation) has claimed the barrier with TryClaim(), it is totally safe to for it perform modifications until Release()
The barrier can be claimed only once and no modifications can be performed without claiming it, so we have no ambiguity on how to interpret Expire() during NotifySuspended()
If TryClaim() returned false we have to discard our operation, no exceptions, if it returned true, we can expect the transaction to be in a clean state - no one modified it before us since registering in the blocking controller

With those guarantees we also don't need EXPIRED_Q and COORD_BLOCKED

Linked:

fix: Fix deadlock in the transaction code. #956 (fixes concurrent data access instead of exclusivity of operations)
Remove blpop FindFirst hop after wakeup #1168 (Adds exclusivity for NotifySuspended(), but does it unsafely)

romange · 2024-01-31T17:54:50Z

src/server/transaction.cc

+}
+
+bool Transaction::SingleClaimBarrier::TryClaim() {
+  return !claimed_.exchange(true, memory_order_relaxed);  // false means first means success


why relaxed?

I am trying to understand why relaxed is enough here. Do not see any obvious reasons why not and it makes me nervous. Usually, it should not be like this.

It was relaxed to begin with 😆

Currently code touches only shard local state or expire/cancel are placed on the coordinator thread

If we acquire, what is our corresponding release pair? We had an acuiqre/release for the hop that prepared suspension, since then we did no writes to our local state, so there is noting to sync

I can make it acquire because though I assume it might have real impact on the conclusion writes

Will think about it

romange · 2024-01-31T18:08:23Z

src/server/transaction.cc

@@ -1470,7 +1484,10 @@ void Transaction::FinishLogJournalOnShard(EngineShard* shard, uint32_t shard_cnt
 }

 void Transaction::CancelBlocking(std::function<OpStatus(ArgSlice)> status_cb) {
-  if ((coordinator_state_ & COORD_BLOCKED) == 0)
+  // We're on the owning thread of this transaction, so accessing local data is safe.


what do you mean by local data here?

Need to phrase it more clearly. My thought is that we're on the owning thread and we don't suspend, so we can for example read keys as we do below. Of course just accessing transaction state wouldn't be allowed for anyone else

src/server/transaction.cc

dranikpg · 2024-01-31T18:50:33Z

src/server/transaction.cc

+}
+
+bool Transaction::SingleClaimBarrier::TryClaim() {
+  return !claimed_.exchange(true, memory_order_relaxed);  // false means first means success


It was relaxed to begin with 😆

Currently code touches only shard local state or expire/cancel are placed on the coordinator thread

If we acquire, what is our corresponding release pair? We had an acuiqre/release for the hop that prepared suspension, since then we did no writes to our local state, so there is noting to sync

I can make it acquire because though I assume it might have real impact on the conclusion writes

Will think about it

src/server/transaction.cc

dranikpg · 2024-01-31T18:53:04Z

src/server/transaction.cc

+}
+
+cv_status Transaction::SingleClaimBarrier::Wait(time_point tp) {
+  auto cb = [this] { return released_.load(memory_order_acquire); };


Same here, I have an acquire here because currently our event-count doesn't provide us with one on the fast path

dranikpg · 2024-01-31T18:55:17Z

src/server/transaction.cc

@@ -1470,7 +1484,10 @@ void Transaction::FinishLogJournalOnShard(EngineShard* shard, uint32_t shard_cnt
 }

 void Transaction::CancelBlocking(std::function<OpStatus(ArgSlice)> status_cb) {
-  if ((coordinator_state_ & COORD_BLOCKED) == 0)
+  // We're on the owning thread of this transaction, so accessing local data is safe.


Need to phrase it more clearly. My thought is that we're on the owning thread and we don't suspend, so we can for example read keys as we do below. Of course just accessing transaction state wouldn't be allowed for anyone else

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg · 2024-02-01T08:44:49Z

I've ran sidekiq loadtests on graviton, seems to pass

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

romange · 2024-02-01T15:46:09Z

src/server/transaction.h

+  // "Single claim - single modification" barrier. Multiple threads might try to claim it, only one
+  // will succeed and will be allowed to modify the guarded object until it closes the barrier.
+  // A closed barrier can't be claimed again or re-used in any way.
+  class SingleClaimBarrier {


totally subjective - maybe to call it BatonBarrier ?
i.e.

Indeed, I only know of folly::Baton and it's somewhat different, but in some way it makes sense and is a more entertaining name 😆

src/core/interpreter.cc

romange · 2024-02-01T16:38:36Z

src/server/transaction.h

-  std::atomic_uint32_t wakeup_requested_{0};  // incremented when blocking transaction gets notified
-  EventCount blocking_ec_;                    // to wait for wakeup_requested > 0 (or cancelled)
+  // Barrier for waking blocking transactions that ensures exclusivity of waking operation.
+  BatonBarrierrier blocking_barrier_{};


something is wrong here. Also maybe "baton_barrier_" ?

Baton barrier doesn't indicate where its used, it like calling an atomic variable just atomic.

blocking_ec just became blocking_barrier... how else can we call blocking transactions? Suspension_barrier, watch_barrier, notify_barrier?

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

romange reviewed Jan 31, 2024

View reviewed changes

dranikpg commented Jan 31, 2024

View reviewed changes

fix(transaction): Add special barrier for blocking tx

396ca3d

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the tx-blocking-barrier branch from 2283d8d to 396ca3d Compare January 31, 2024 19:00

dranikpg marked this pull request as ready for review February 1, 2024 08:23

fix: fixes and comments

f8a8e76

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the tx-blocking-barrier branch from 577cc6c to f8a8e76 Compare February 1, 2024 08:50

dranikpg requested a review from romange February 1, 2024 08:51

romange reviewed Feb 1, 2024

View reviewed changes

romange previously approved these changes Feb 1, 2024

View reviewed changes

dranikpg dismissed romange’s stale review via 62f804a February 1, 2024 16:23

romange reviewed Feb 1, 2024

View reviewed changes

src/core/interpreter.cc Outdated Show resolved Hide resolved

romange reviewed Feb 1, 2024

View reviewed changes

src/core/interpreter.cc Outdated Show resolved Hide resolved

romange reviewed Feb 1, 2024

View reviewed changes

chore: rename to Baton

80b532f

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the tx-blocking-barrier branch from 62f804a to 80b532f Compare February 1, 2024 17:26

dranikpg requested a review from romange February 1, 2024 17:28

romange approved these changes Feb 1, 2024

View reviewed changes

dranikpg merged commit 40d08a3 into dragonflydb:main Feb 1, 2024
7 checks passed

dranikpg deleted the tx-blocking-barrier branch February 6, 2024 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(transaction): Add special barrier for blocking tx #2512

fix(transaction): Add special barrier for blocking tx #2512

dranikpg commented Jan 31, 2024 •

edited

Loading

romange Jan 31, 2024

romange Jan 31, 2024

dranikpg Jan 31, 2024

romange Jan 31, 2024

dranikpg Jan 31, 2024

dranikpg Jan 31, 2024

dranikpg Jan 31, 2024 •

edited

Loading

dranikpg Jan 31, 2024

dranikpg commented Feb 1, 2024

romange Feb 1, 2024 •

edited

Loading

dranikpg Feb 1, 2024

romange Feb 1, 2024

dranikpg Feb 1, 2024

fix(transaction): Add special barrier for blocking tx #2512

fix(transaction): Add special barrier for blocking tx #2512

Conversation

dranikpg commented Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg commented Feb 1, 2024

romange Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg commented Jan 31, 2024 •

edited

Loading

dranikpg Jan 31, 2024 •

edited

Loading

romange Feb 1, 2024 •

edited

Loading