fix: data race in save_stages_controller #2647

kostasrim · 2024-02-22T16:25:12Z

Initialize resources atomically to avoid race condition
Finalize/cleanup resources atomically to avoid race condition
Refactor SaveStagesController::Save

kostasrim · 2024-02-22T16:48:53Z

src/server/server_family.cc

  }

-  detail::SaveInfo save_info = save_controller_->Save();
+  save_controller_->WaitAllSnapshots();


Bonus points because now the function clearly suggests what's happening under the hood. @romange we can turn this function async easily. We could add a background fiber for that, which would turn BGSAVE from synchronous to asynchronous (bringing us on par with redis). This is not a priority but since it was mentioned in the issues I am bringing it up :)

ok, this answers my first question.

kostasrim · 2024-02-22T16:51:12Z

src/server/detail/save_stages_controller.cc


+SaveInfo Finalize() {
  RunStage(&SaveStagesController::CloseCb);


This should be done atomically as well

dranikpg

If to follow it fully, the mechanism behind GetTotalBuffersSize() is quite complicated

For DFS we dispatch immediately from SaveStagesController and assume we're placed on each shard correctly

For RDB we call RdbSnapshot::Impl::GetSaveBuffersSize transparently and dispatch only there on all the snapshot threads

So we have branching twice, but on different levels 🤯

Left some nits

dranikpg · 2024-02-22T17:50:30Z

src/server/detail/save_stages_controller.h

  SaveStagesController(SaveStagesInputs&& input);
+  std::optional<SaveInfo> InitResourcesAndStart();

  ~SaveStagesController();

-  SaveInfo Save();
+  void WaitAllSnapshots();
+
+  SaveInfo Finalize();


Whenever a type has a multi-step interface, please provide proper comments on how to use it correctly (and why actually those are different steps).

If people won't find comments, they'll misuse it very quickly. Proven by experience 😆

Especially for a type that has outside synchronization which is sometimes difficult to follow. I'd make the case for internal synchronization, but we also need external for updating the pointer, so it's not worth dividing those

Also comment what returning optional means.... It's not evident that this is an error in fact

I agree, the interface a bit non-intuitive. I also agree that this PR should not do lots of changes.

I added comments. I wanted to push a refactor as I wrote somewhere else but I thought for not it's best to only fix the issue (even if it's not perfect)

dranikpg · 2024-02-22T17:54:58Z

src/server/server_family.cc

+  auto set_last_save_error = [this](const auto& save_info) mutable {
+    last_save_info_.last_error = save_info.error;
+    last_save_info_.last_error_time = save_info.save_time;
+    last_save_info_.failed_duration_sec = save_info.duration_sec;
+  };


nit: If to speak strictly, it should really be an function on last safe info 😅 Actually, we have two types LastSaveInfo and SaveInfo, whereas the former should clearly be a subclass of the latter. We should add it as a refactoring todo (if nobody objects 😼)

I agree and since it's a very quick change, I did it now :) The only todo I will leave is a major TODO :refactor 🤣

dranikpg · 2024-02-22T17:57:08Z

src/server/detail/save_stages_controller.cc

-    return GetSaveInfo();
+    SaveInfo save = GetSaveInfo();
+    return std::optional<SaveInfo>(std::move(save));


Nit: I'm almost sure return GetSaveInfo() works as well

On my defense, I was experimenting with a false positive from you complete me. Basically I had it like this and I got a warning on Vim so I changed it because I was curious if the warning would go away (it didn't) and I left forgotten as is 👀

romange · 2024-02-22T18:17:50Z

src/server/server_family.cc

+    auto res = save_controller_->InitResourcesAndStart();
+
+    if (res) {
+      CHECK_EQ(res->error, true);


if this invariant won't hold will we reach UB? I do not think so.
If not, we should replace CHECK with DCHECK - to reduce the risk of crashing because of minor bugs.

see why arrogant checks are evil: #2649

It asserts interface compatibility. The best way to do it is with proper typing, i.e. nonstd::expected. If this path is not covered in tests, the DCHECK will be never checked and we'll get puzzling behaviour in case someone breaks the interface. But I'd also favour a DCHECK (if it ever runs), in the future we should look at #2647 (comment)

made it a DCHECK

romange · 2024-02-22T18:17:56Z

src/server/server_family.cc

    save_controller_ = make_unique<SaveStagesController>(detail::SaveStagesInputs{
        new_version, basename, trans, &service_, fq_threadpool_.get(), snapshot_storage_});
+
+    auto res = save_controller_->InitResourcesAndStart();


Why not have "Init()" function that can fail and then keep Save() function that does Start and Wait as before?

This is exactly what we do. But I assume you know this from:

ok, this answers my first question.

needs fixes

dranikpg

Please wait to see if Roman has any objections because I'm as usually an early-approver 😆

dranikpg · 2024-02-23T13:41:52Z

src/server/detail/save_stages_controller.h

+  // Objects of this class are used concurrently. Call this function
+  // in a mutually exlusive context to avoid data races.
+  // Also call this function before any call to `WaitAllSnapshots`
+  // Returns empty optional on success and SaveInfo on failure
+  std::optional<SaveInfo> InitResourcesAndStart();


It could be a little more specific (what means used concurrently) like: the save controller can be queried for stats from another threads at any time, so all setup and teardown operations must be guarded, etc...

* fix: data race in save_stages_controller

kostasrim added 2 commits February 22, 2024 18:23

fix: data race in save_stages_controller

5dc7c14

remove unused absl::cleanup

f538ad9

kostasrim self-assigned this Feb 22, 2024

fix race on cleanup

9b909e5

kostasrim commented Feb 22, 2024

View reviewed changes

kostasrim mentioned this pull request Feb 22, 2024

feat: add current_fork_perc in info all command #2640

Merged

kostasrim commented Feb 22, 2024

View reviewed changes

fix build

34ba388

kostasrim requested review from romange and dranikpg February 22, 2024 17:33

dranikpg previously approved these changes Feb 22, 2024

View reviewed changes

romange reviewed Feb 22, 2024

View reviewed changes

address gh comments

efcc91a

kostasrim requested review from romange and dranikpg February 23, 2024 09:23

add more comments

172c920

dranikpg approved these changes Feb 23, 2024

View reviewed changes

romange approved these changes Feb 23, 2024

View reviewed changes

romange merged commit 3d27913 into main Feb 23, 2024
10 checks passed

romange deleted the fix_info_all_race branch February 23, 2024 15:35

romange pushed a commit that referenced this pull request Feb 23, 2024

fix: data race in save_stages_controller (#2647)

a5af87e

* fix: data race in save_stages_controller

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: data race in save_stages_controller #2647

fix: data race in save_stages_controller #2647

kostasrim commented Feb 22, 2024 •

edited

kostasrim Feb 22, 2024 •

edited

romange Feb 22, 2024

kostasrim Feb 22, 2024

dranikpg left a comment

dranikpg Feb 22, 2024

dranikpg Feb 22, 2024

dranikpg Feb 22, 2024

romange Feb 22, 2024

kostasrim Feb 23, 2024

dranikpg Feb 22, 2024

kostasrim Feb 23, 2024

dranikpg Feb 22, 2024

kostasrim Feb 23, 2024

romange Feb 22, 2024

romange Feb 22, 2024

dranikpg Feb 22, 2024 •

edited

kostasrim Feb 23, 2024

romange Feb 22, 2024

kostasrim Feb 23, 2024

dranikpg left a comment

dranikpg Feb 23, 2024


		SaveInfo Finalize() {
		RunStage(&SaveStagesController::CloseCb);

fix: data race in save_stages_controller #2647

fix: data race in save_stages_controller #2647

Conversation

kostasrim commented Feb 22, 2024 • edited

kostasrim Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg Feb 22, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Feb 22, 2024 •

edited

kostasrim Feb 22, 2024 •

edited

dranikpg Feb 22, 2024 •

edited