fix(server): client pause work while blocking commands run #2584

adiholden · 2024-02-13T13:03:02Z

fix #2576
fix #2661

dranikpg

I see, we can ignore them because no updates are possible, they can't be unlocked.

The only issue I see:

While we wait with the dispatch tracker, some command unblocks a blocking one
The blocking one starts doing stuff, but we didn't track it, so we don't wait for it

adiholden · 2024-02-13T16:51:21Z

@dranikpg you are right. Have a suggestion how we can fix this?

dranikpg · 2024-02-14T08:38:46Z

@adiholden My only idea would be to call it again, maybe we can do it only if we have blocking commands.

Run dispatch tracker
Wait for non-blocking to finish
All blocked commands should be running within some suspension points, milliseconds (?) 🤷🏻‍♂️ Maybe we can track them somehow, like with blockingcontroller->awakened
Run dispatch tracker again to catch all non blocking ones

so maybe

Dispatch {  
  tracker.Track()
}
tracker.Wait()

while Dispatch { blocking_controller -> queue_map > 0 }
  sleep x ms

Dispatch {  tracker.Track() } // wait for two-hop commands like brpoplpush to finish
tracker.Wait()

dranikpg · 2024-02-14T08:39:57Z

We also have one more problem, namely BLPOP on it's started phase is not counted as blocking, it has two hops:

Find key
-- we use Tracker.Track() here, it doesn't recognize it as blocked, but it will block --
WaitOnWatch

Will report on this as well 🤔🤔🤔🤔

adiholden · 2024-02-14T09:28:59Z

We also have one more problem, namely BLPOP on it's started phase is not counted as blocking, it has two hops:

Find key
-- we use Tracker.Track() here, it doesn't recognize it as blocked, but it will block --

WaitOnWatch

Will report on this as well 🤔🤔🤔🤔

So actually having 2 hops in blpop also makes CancelBlocking funciton bugy

dranikpg · 2024-02-16T06:04:01Z

Maybe it's simpler just to do busy looping

Dispatch { paused = true }
do {
  ok = Dispatch {
    if (num_dispatch - num_blocking - num_paused > 0) return false
    if (blocking_controller -> awakened > 0) return false
    return true;
  }
  if (!ok) sleep(1ms)
} while (!ok)

The difference here is that we retry this condition, so we wait for BLPOP to either finish or become blocked, so the difference = 0

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden · 2024-02-26T14:56:55Z

src/server/main_service.cc

@@ -1327,7 +1327,9 @@ size_t Service::DispatchManyCommands(absl::Span<CmdArgList> args_list,
    // paired with shardlocal eval
    const bool is_eval = CO::IsEvalKind(ArgS(args, 0));

-    if (!is_multi && !is_eval && cid != nullptr) {
+    const bool is_blocking = cid != nullptr && cid->IsBlocking();


This is a fix for #2661

Thanks 🙏🏻

adiholden · 2024-02-26T15:03:32Z

@dranikpg I pushed a fix which will solve both of the seraios you mentioned above:

if a blocking transaction becomes unblocked is solved by running AwaitPauseState inside Transaction::WaitOnWatch after we are waking up from blocking
the 2 hops problem with the blpop is fixed by running the tracker twice if we reached the timeout, as at the first time we run the TrackAll the transaction is blocking and we will not send a checkpoint

dranikpg · 2024-02-26T15:50:20Z

will review in 30 mins

dranikpg

I suggest to merge it. With a very low probably we can time out spuriously or wait one second... But most importantly it looks correct

dranikpg · 2024-02-26T15:15:32Z

src/server/main_service.cc

@@ -1327,7 +1327,9 @@ size_t Service::DispatchManyCommands(absl::Span<CmdArgList> args_list,
    // paired with shardlocal eval
    const bool is_eval = CO::IsEvalKind(ArgS(args, 0));

-    if (!is_multi && !is_eval && cid != nullptr) {
+    const bool is_blocking = cid != nullptr && cid->IsBlocking();


Thanks 🙏🏻

dranikpg · 2024-02-26T15:17:12Z

src/facade/dragonfly_listener.cc

@@ -392,6 +392,7 @@ DispatchTracker::DispatchTracker(absl::Span<facade::Listener* const> listeners,
      issuer_{issuer},
      ignore_paused_{ignore_paused},
      ignore_blocked_{ignore_blocked} {
+  bc_ = make_unique<util::fb2::BlockingCounter>(0);


It's already a smart pointer internally, you can just re-assign it

dranikpg · 2024-02-26T15:40:25Z

src/facade/dragonfly_listener.cc

+  bool res = bc_->WaitFor(absl::ToChronoMilliseconds(duration));
+  if (!res && ignore_blocked_) {
+    // We track all connections again because a connection might became blocked between the time
+    // we call tracking the last time.
+    bc_ = make_unique<util::fb2::BlockingCounter>(0);
+    TrackAll();
+    res = bc_->WaitFor(absl::ToChronoMilliseconds(duration));
+  }
+  return res;


so whenever we encounter such a case we time out on the 1 second which is somewhat unpleasant, but at least we don't miss it 🙂

Theoretically, sometimes, (say, a few ms timeout and a packed transaction queue, it is not guaranteed, that all blocking commands reach a suspension point, so we can spuriously time out again.

I mean that blpop schedules, wait for the first hop, we hit the first timeout, wait again and only at that point blpop finishes the first hop and suspends (yet we've send it a checkpoint)

dranikpg · 2024-02-26T15:43:44Z

src/server/server_family.cc

-  DispatchTracker tracker{listeners, conn, true /* ignore paused commands */};
+  DispatchTracker tracker{listeners, conn, true /* ignore paused commands */,
+                          true /*ignore blocking*/};


We should comment why we ignore blocking commands and how they are handled

Namely that whenver a blocking command wakes up it first checks the pause state, so we don't have to track them

dranikpg · 2024-02-26T16:38:13Z

src/server/transaction.cc

  // Wait for the blocking barrier to be closed.
  // Note: It might return immediately if another thread already notified us.
  cv_status status = blocking_barrier_.Wait(tp);

  DVLOG(1) << "WaitOnWatch done " << int(status) << " " << DebugId();
  --stats->num_blocked_clients;

+  // TBD set connection pause state


Yes, otherwise it's a deadlock 😅

We should also document the reasoning somewhere, it's not evident:

Because we set paused atomically and immediately track running commands, it should be correct.

If we wake up a thread that wasn't tracked yet, it's also not paused yet, so we'll see the woken up command as running and we'll track it

If we wake up a thread that is already paused, it will pause upon unblocking, so no data operations will be performed

dranikpg · 2024-02-26T16:44:47Z

tests/dragonfly/connection_test.py

+
+
+@pytest.mark.asyncio
+async def test_blocking_command_client_pause(async_client: aioredis.Redis):


dranikpg · 2024-02-26T16:56:06Z

But please add a comment above pause why it works with blocking, even though it ignores them 😅

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix #2576 fix #2661 Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested review from BorysTheDev and dranikpg February 13, 2024 13:03

dranikpg previously approved these changes Feb 13, 2024

View reviewed changes

BorysTheDev previously approved these changes Feb 13, 2024

View reviewed changes

romange changed the title ~~fix(server): client puase work while blocking commands run~~ fix(server): client pause work while blocking commands run Feb 15, 2024

adiholden added 2 commits February 22, 2024 13:20

fix(server): client puase work while blocking commands run

a03cd88

Signed-off-by: adi_holden <adi@dragonflydb.io>

fix PR

396a7cd

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden dismissed stale reviews from BorysTheDev and dranikpg via 396a7cd February 26, 2024 14:55

adiholden force-pushed the client_pause_ignore_blocking branch from 74229e1 to 396a7cd Compare February 26, 2024 14:55

adiholden commented Feb 26, 2024

View reviewed changes

BorysTheDev previously approved these changes Feb 26, 2024

View reviewed changes

dranikpg previously approved these changes Feb 26, 2024

View reviewed changes

fix PR comments

a5e1b71

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden dismissed stale reviews from dranikpg and BorysTheDev via a5e1b71 February 28, 2024 10:40

adiholden requested a review from dranikpg February 28, 2024 10:43

dranikpg approved these changes Feb 28, 2024

View reviewed changes

adiholden enabled auto-merge (squash) February 28, 2024 10:51

adiholden merged commit 7e45270 into main Feb 28, 2024
10 checks passed

adiholden deleted the client_pause_ignore_blocking branch February 28, 2024 11:07

romange pushed a commit that referenced this pull request Mar 1, 2024

fix(server): client pause work while blocking commands run (#2584)

9ce9580

fix #2576 fix #2661 Signed-off-by: adi_holden <adi@dragonflydb.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): client pause work while blocking commands run #2584

fix(server): client pause work while blocking commands run #2584

adiholden commented Feb 13, 2024 •

edited

Loading

dranikpg left a comment

adiholden commented Feb 13, 2024

dranikpg commented Feb 14, 2024

dranikpg commented Feb 14, 2024

adiholden commented Feb 14, 2024

dranikpg commented Feb 16, 2024 •

edited

Loading

adiholden Feb 26, 2024

dranikpg Feb 26, 2024

adiholden commented Feb 26, 2024

dranikpg commented Feb 26, 2024

dranikpg left a comment

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg Feb 26, 2024

dranikpg commented Feb 26, 2024



		@pytest.mark.asyncio
		async def test_blocking_command_client_pause(async_client: aioredis.Redis):

fix(server): client pause work while blocking commands run #2584

fix(server): client pause work while blocking commands run #2584

Conversation

adiholden commented Feb 13, 2024 • edited Loading

dranikpg left a comment

Choose a reason for hiding this comment

adiholden commented Feb 13, 2024

dranikpg commented Feb 14, 2024

dranikpg commented Feb 14, 2024

adiholden commented Feb 14, 2024

dranikpg commented Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiholden commented Feb 26, 2024

dranikpg commented Feb 26, 2024

dranikpg left a 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

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 commented Feb 26, 2024

adiholden commented Feb 13, 2024 •

edited

Loading

dranikpg commented Feb 16, 2024 •

edited

Loading