fix: ub in RegisterOnChange and regression tests for big values #3336

kostasrim · 2024-07-18T12:44:11Z

I renamed the flag but I never renamed it in the tests

The problem is that if we start iterating over db_slice::change_cb_ and we preempt, then a RegisterOnChange might invalidate the iterators of the change_cb_ (since it's a vector) leading to UB. The fix is to coordinate RegisterOnChange and register a new callback only after all the fibers stop iterating over the vector.

fix replication test flag name for big values
fix a bug that triggers ub when RegisterOnChange is called on flows that iterate over the callbacks and preempt
add a stress test for big value serialization

kostasrim · 2024-07-18T12:44:58Z

reg test run https://github.com/dragonflydb/dragonfly/actions/runs/9991741595

kostasrim · 2024-07-18T18:18:42Z

src/server/db_slice.cc

  for (const auto& ccb : change_cb_) {
+    CHECK(ccb.second);


sanity, cause it helped me identify the problem. UB is ub which doesn't gurantee this check, but it's free and you never know :)

kostasrim · 2024-07-18T18:22:36Z

another one https://github.com/dragonflydb/dragonfly/actions/runs/9996620325

romange · 2024-07-18T18:29:42Z

src/server/db_slice.h

@@ -524,6 +525,39 @@ class DbSlice {
  void CallChangeCallbacks(DbIndex id, const ChangeReq& cr) const;

 private:
+  struct CounterCondFlag {


I have some feedback about naming.
we already have BlockingCounter which has the same semantics. why not call it LocalBlockingCounter since it's just a single threaded version of the same class.

We currently use just a SharedMutex in the journal for the same problem. We can add a non-atomic mutex, switch to properly using lists or add this construct for both cases

I will take care of this in separate PR. Nice @dranikpg

#3337 so I don't forget

I actually think there is even more elegant solution to that that will void the need in any locking at all.
I think we can think of adding a quiescent notion to the transactional framework that will make sure calling callbacks during "safe" periods

@romange happy to take this (but let's defer this until we get the other staff done). Can you plz expand your idea? I will create an issue to remember this :)

Thought about it more. My approach won't work here because RegisterOnChange returns a version

romange · 2024-07-18T18:30:13Z

src/server/db_slice.h

@@ -524,6 +525,39 @@ class DbSlice {
  void CallChangeCallbacks(DbIndex id, const ChangeReq& cr) const;

 private:
+  struct CounterCondFlag {
+    void WaitUntilCounterIsZero() {


maybe Wait or WaitZero if Counter is part of the class name?

romange · 2024-07-18T18:32:01Z

src/server/db_slice.h

+    size_t mutating = 0;
+  };
+
+  class CounterConditionGuard {


if you add equivalent lock, unlock methods to LocalBlockingCounter you will be able to use std::lock_guard instead of writing a custom class.

Feels like hijacking the interface of the mutex/lock_guard since on lock we should not really lock anything. We only block on certain codepaths so std::unique_lock kinda acts as a counter now. I made the change though including the renames you suggested above :)

dranikpg · 2024-07-18T20:25:33Z

I edited the code recently. So with support for big value serialization we do suspend now? Because previously we had atomic guards in all the callbacks

kostasrim · 2024-07-18T20:58:15Z

I edited the code recently. So with support for big value serialization we do suspend now? Because previously we had atomic guards in all the callbacks

yes! But it;s safe, Shahar implemented the laundering iterator so preemptions won't harm us :)

chakaz

LG, small nits

chakaz · 2024-07-21T07:11:28Z

src/server/db_slice.h

+    util::fb2::CondVarAny cond_var;
+    size_t mutating = 0;


Please end var names with _, i.e. cond_var_ and _mutating_

chakaz · 2024-07-21T07:13:59Z

src/server/db_slice.h

+    }
+
+    void unlock() {
+      --mutating;


Please add DCHECK(mutating > 0); here

fix: rename big value limit flag in replication test

20b220f

kostasrim requested a review from chakaz July 18, 2024 12:44

kostasrim self-assigned this Jul 18, 2024

chakaz previously approved these changes Jul 18, 2024

View reviewed changes

fixes

823db72

kostasrim dismissed chakaz’s stale review via 823db72 July 18, 2024 18:14

kostasrim changed the title ~~fix: rename big value limit flag in replication test~~ fix: ub in RegisterOnChange and regression tests for big values Jul 18, 2024

kostasrim requested a review from chakaz July 18, 2024 18:16

kostasrim commented Jul 18, 2024

View reviewed changes

romange reviewed Jul 18, 2024

View reviewed changes

address comments

48781aa

kostasrim requested review from romange and dranikpg July 18, 2024 20:58

kostasrim mentioned this pull request Jul 18, 2024

Replace SharedMutex in Journal #3337

Open

dranikpg approved these changes Jul 19, 2024

View reviewed changes

kostasrim enabled auto-merge (squash) July 19, 2024 06:48

kostasrim merged commit 8a2d6ad into main Jul 19, 2024
20 checks passed

kostasrim deleted the fix_flag_in_tests branch July 19, 2024 07:03

chakaz reviewed Jul 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ub in RegisterOnChange and regression tests for big values #3336

fix: ub in RegisterOnChange and regression tests for big values #3336

kostasrim commented Jul 18, 2024 •

edited

Loading

kostasrim commented Jul 18, 2024

kostasrim Jul 18, 2024 •

edited

Loading

kostasrim commented Jul 18, 2024

romange Jul 18, 2024

dranikpg Jul 18, 2024

kostasrim Jul 18, 2024

kostasrim Jul 18, 2024

romange Jul 19, 2024

kostasrim Jul 19, 2024

romange Jul 21, 2024

romange Jul 18, 2024

romange Jul 18, 2024

kostasrim Jul 18, 2024

dranikpg commented Jul 18, 2024

kostasrim commented Jul 18, 2024

chakaz left a comment

chakaz Jul 21, 2024

chakaz Jul 21, 2024

fix: ub in RegisterOnChange and regression tests for big values #3336

fix: ub in RegisterOnChange and regression tests for big values #3336

Conversation

kostasrim commented Jul 18, 2024 • edited Loading

kostasrim commented Jul 18, 2024

kostasrim Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

kostasrim commented Jul 18, 2024

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 Jul 18, 2024

kostasrim commented Jul 18, 2024

chakaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Jul 18, 2024 •

edited

Loading

kostasrim Jul 18, 2024 •

edited

Loading