fix: race condition in ReplicaOfInternal #4653

kostasrim · 2025-02-24T14:36:11Z

#4636 introduced a small race . The problem is that Start() might have failed (because a subsequent ReplicaOf command cancelled it). ReplicaOfInternal would ignore the ec and start the replication fiber via replica_->StartMainReplicationFiber but the second ReplicaOf command that followed updated replica_ so we tried to start replication twice on the same replica instance triggering a check failed that the fiber is running.

We did not have that problem before because we called new_replica_->Start() which would start replication. Even if the replication got cancelled because of a second ReplicaOf command, it would be fine because the MainReplicationFiber would start twice on two separate Replica instances and one of them would cancel right away becauseR_CANCELLED was not set (since Stop() explicitly set it by cancelling the context)

Signed-off-by: kostas <kostas@dragonflydb.io>

kostasrim · 2025-02-24T14:44:00Z

src/server/server_family.cc

@@ -2845,7 +2851,7 @@ void ServerFamily::ReplicaOfInternal(CmdArgList args, Transaction* tx, SinkReply
  }

  if (on_err == ActionOnConnectionFail::kReturnOnError) {
-    replica_->StartMainReplicationFiber(builder);
+    new_replica->StartMainReplicationFiber(builder);


This is the FIX. Same replica_ instance was used by two fibers and we ended up starting the main replication fiber twice. The rest of the changes above are an early exit to avoid creating the fiber all together in case of an error or if the context got cancelled. This small optimization is not really needed even in the presence of parallel ReplicaOf commands because MainReplicationFiber exits early if the context is cancelled (that's why we did not need to check for ec before my changes) -- so as long as this replication fiber is part of the correct instance -- cancellation would take care of the rest.

kostasrim · 2025-02-24T14:45:06Z

src/server/server_family.cc

+  // However, it could be the case that Start() above connected succefully and by the time
+  // we acquire the lock, the context got cancelled because another ReplicaOf command
+  // executed and acquired the replicaof_mu_ before us.
+  if (ec || new_replica->IsContextCancelled()) {


Small optimization. We do not need to start the replication fiber if we got an error or if the context is cancelled.

This would be handled anyway by new_replica->StartMainReplicationFiber which would spawn the MainReplicationFiber and then exit because it got cancelled. But yeah, no reason to do that

kostasrim · 2025-02-24T15:04:43Z

https://github.com/dragonflydb/dragonfly/actions/runs/13516050833 🟢

BorysTheDev · 2025-02-24T20:01:47Z

src/server/replica.h

@@ -72,6 +72,10 @@ class Replica : ProtocolClient {

  std::error_code TakeOver(std::string_view timeout, bool save_flag);

+  bool IsContextCancelled() const {
+    return !cntx_.IsRunning();


Do you really need such a function

We can just start the replication fiber and let it cancel. I wanted to avoid that alltogether so I added this. As I explained, this is not part of the fix but an "early optimization" (which is kinda funny because fiber cost is insignificant and this path is non critical)

BorysTheDev · 2025-02-24T20:04:27Z

src/server/server_family.cc

+  // we acquire the lock, the context got cancelled because another ReplicaOf command
+  // executed and acquired the replicaof_mu_ before us.
+  if (ec || new_replica->IsContextCancelled()) {
+    if (replica_ == new_replica) {


what if replica != new_replica for the first command and the second command was cancelled. In this case we will not have replica at all and botjh commands are failed. Maybe lets discuss it with @adiholden

Then we should not have a replica! The second command cancelled the previously then it cancelled itself ? Isn't that what you are describing or am I missing something ?

adiholden · 2025-02-25T09:51:14Z

src/server/server_family.cc

+  // we acquire the lock, the context got cancelled because another ReplicaOf command
+  // executed and acquired the replicaof_mu_ before us.
+  if (ec || new_replica->IsContextCancelled()) {
+    if (replica_ == new_replica) {


how can replica_ == new_replica if new_replica->IsContextCancelled()?
when we cancel replication (from replica of command)
we take replicaof mutex
call replica_->Stop
and set replica_ var
So from what I understadn we can not get to the condition replica_ == new_replica and new_replica->IsContextCancelled()

Another comment here.
I think that if replica was canceld and ec is not error you do not return response to client.

So from what I understadn we can not get to the condition replica_ == new_replica and new_replica->IsContextCancelled()

Yes that's true and it is not an issue. The IsContextCancelled() is ored with ec because ec might be 0 (meaning success) but the context got cancelled. For that case we just return. However, if ec has an error and replica == new_replica then it means we need to cancel everything alltogether (there is no other replica of command and we just cancelled ourselves)

+1 for the missing response

adiholden · 2025-02-25T09:52:12Z

src/server/server_family.cc

@@ -2845,7 +2851,7 @@ void ServerFamily::ReplicaOfInternal(CmdArgList args, Transaction* tx, SinkReply
  }


It will be more clear I think if you enter the Drakarys call under if (on_err == ActionOnConnectionFail::kReturnOnError)instead of if (tx)

I did not want to change the semantics. I will update on this

Yep +1 for the suggestion. This also removes tx alltogether as well

oh Drakarys needs, nvm

Signed-off-by: kostas <kostas@dragonflydb.io>

adiholden · 2025-02-26T07:24:14Z

src/server/server_family.cc

+      SetMasterFlagOnAllThreads(true);
+      replica_.reset();
+    }
+    // SendError if we got cancelled and Start() above succeeded. Otherwise, if ec,


This is confusing. Please send the error from one place here. lets not send it in start but only here.

I suggest to write like this
if (ec && replica_ == new_replica) {
service_.SwitchState(GlobalState::LOADING, GlobalState::ACTIVE);
SetMasterFlagOnAllThreads(true);
replica_.reset();
}
if (ec || cancelled) {
builder->SendError
return;
} else {
builder->SendOk();
}
if (on_err == ActionOnConnectionFail::kReturnOnError) {
Drakarys(tx, DbSlice::kDbAll);
new_replica->StartMainReplicationFiber(builder);
}

adiholden · 2025-02-26T07:25:27Z

src/server/server_family.cc

  if (on_err == ActionOnConnectionFail::kReturnOnError) {
-    replica_->StartMainReplicationFiber(builder);
+    Drakarys(tx, DbSlice::kDbAll);
+    new_replica->StartMainReplicationFiber(builder);


I suggest to send the ok also here and not inside StartMainReplicationFiber

adiholden · 2025-02-26T08:38:42Z

src/server/replica.cc

  VLOG(1) << "Starting replication";
  ProactorBase* mythread = ProactorBase::me();
  CHECK(mythread);

-  auto check_connection_error = [this, builder](error_code ec, const char* msg) -> error_code {
+  auto check_connection_error = [this](error_code ec, const char* msg) -> GenericError {


can RETURN_ON_ERR work on GenericError?

Generic error is implicitly convertible to error code. However RETURN_ON_ERR does this:

GenericError -> ErrorCode then RETURN_ON_ERR returns that ErrorCode, which is then converted back to GenericError.

This looses the details part of the GenericError. I will fix this.

I am SAVING my rant around implicit conversions.

adiholden · 2025-02-26T08:40:11Z

src/server/replica.cc

      cntx_.ReportCancelError();
+      return {{}, absl::StrCat(msg, ec.message())};


I am confused how this is working.. why shouldnt this be return {ec, absl::StrCat(msg, ec.message())}

Look how GenericError::Format is implemented. The above expression is turned to absl::StrCat(msg, ec.message() when formatted.

Also we can construct GenericError directly from string. We can omit the first argument {} because we have that constructor. I will take care of this as well

adiholden · 2025-02-26T09:51:36Z

src/server/replica.cc

      cntx_.ReportCancelError();
+      return {absl::StrCat(msg, ec.message())};


Because the GenericError does this concatenation for us you can do
return {ec, msg}

It does not do the same concatenation.

330 std::string GenericError::Format() const { 331 if (!ec_ && details_.empty()) 332 return ""; 333 334 if (details_.empty()) 335 return ec_.message(); 336 else if (!ec_) 337 return details_; 338 else 339 return absl::StrCat(ec_.message(), ": ", details_); 340 }

ec is true so absl::StrCat(ec_.message(), ": ", details_); is not identical to {absl::StrCat(msg, ec.message())};

do we care about the order here? does this realy needs a different implementaion?

I would need to check but it's irrelevant. At the end of the day we are discussing here return {ec, msg} vs return {msg}. Either way is fine and it's not important

kostasrim · 2025-02-26T11:05:22Z

src/server/error.h

@@ -31,6 +31,14 @@ using facade::kWrongTypeErr;

 #define RETURN_ON_ERR(x) RETURN_ON_ERR_T(std::error_code, x)

+#define RETURN_ON_GENERIC_ERR(x)                                   \


@adiholden

I had to, there is no really good solution otherwise. Doing something RETURN_ON_ERR_T(GenericError, x) won't work. Yes I tried modifying the micro above by converting the statement:

std::error_code __ec = (x); ----> T __ex = (x) but a) this is not properly being parsed and we get compiler errors, b) we also do an extra copy in that expression which is fine for error code because it's trivially copyable but not for Generic error.

kostasrim self-assigned this Feb 24, 2025

fix: race condition in ReplicaOfInternal

Loading
Loading status checks…

5170d3a

Signed-off-by: kostas <kostas@dragonflydb.io>

kostasrim force-pushed the kpr18 branch from ae81571 to 5170d3a Compare February 24, 2025 14:38

kostasrim commented Feb 24, 2025

View reviewed changes

kostasrim requested review from BorysTheDev and adiholden February 24, 2025 14:45

BorysTheDev reviewed Feb 24, 2025

View reviewed changes

adiholden reviewed Feb 25, 2025

View reviewed changes

comments

Loading
Loading status checks…

204fe7b

Signed-off-by: kostas <kostas@dragonflydb.io>

kostasrim requested a review from adiholden February 25, 2025 13:01

adiholden reviewed Feb 26, 2025

View reviewed changes

kostasrim added 2 commits February 26, 2025 10:21

comments

Loading
Loading status checks…

d698ad1

missing return

Loading
Loading status checks…

b8ee2cc

kostasrim requested a review from adiholden February 26, 2025 08:24

adiholden reviewed Feb 26, 2025

View reviewed changes

fixes

Loading
Loading status checks…

fc0fe79

adiholden reviewed Feb 26, 2025

View reviewed changes

fixes

Loading
Loading status checks…

52efea6

kostasrim commented Feb 26, 2025

View reviewed changes

kostasrim requested a review from adiholden February 26, 2025 11:06

adiholden approved these changes Feb 26, 2025

View reviewed changes

kostasrim merged commit 8512f72 into main Feb 26, 2025
10 checks passed

kostasrim deleted the kpr18 branch February 26, 2025 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: race condition in ReplicaOfInternal #4653

fix: race condition in ReplicaOfInternal #4653

kostasrim commented Feb 24, 2025 •

edited

Loading

kostasrim Feb 24, 2025 •

edited

Loading

kostasrim Feb 24, 2025

kostasrim commented Feb 24, 2025 •

edited

Loading

BorysTheDev Feb 24, 2025

kostasrim Feb 25, 2025

BorysTheDev Feb 24, 2025

kostasrim Feb 25, 2025

adiholden Feb 25, 2025

adiholden Feb 25, 2025

kostasrim Feb 25, 2025

kostasrim Feb 25, 2025

adiholden Feb 25, 2025

kostasrim Feb 25, 2025

kostasrim Feb 25, 2025

kostasrim Feb 25, 2025

adiholden Feb 26, 2025

adiholden Feb 26, 2025

adiholden Feb 26, 2025

adiholden Feb 26, 2025

kostasrim Feb 26, 2025 •

edited

Loading

adiholden Feb 26, 2025

kostasrim Feb 26, 2025

kostasrim Feb 26, 2025

adiholden Feb 26, 2025

kostasrim Feb 26, 2025 •

edited

Loading

adiholden Feb 26, 2025

kostasrim Feb 26, 2025 •

edited

Loading

kostasrim Feb 26, 2025 •

edited

Loading

		@@ -2845,7 +2851,7 @@ void ServerFamily::ReplicaOfInternal(CmdArgList args, Transaction* tx, SinkReply
		}

		cntx_.ReportCancelError();
		return {{}, absl::StrCat(msg, ec.message())};

		cntx_.ReportCancelError();
		return {absl::StrCat(msg, ec.message())};

		@@ -31,6 +31,14 @@ using facade::kWrongTypeErr;

		#define RETURN_ON_ERR(x) RETURN_ON_ERR_T(std::error_code, x)

		#define RETURN_ON_GENERIC_ERR(x) \

fix: race condition in ReplicaOfInternal #4653

fix: race condition in ReplicaOfInternal #4653

Conversation

kostasrim commented Feb 24, 2025 • edited Loading

kostasrim Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Feb 24, 2025 • 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

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

kostasrim Feb 26, 2025 • 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

kostasrim Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

kostasrim Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

kostasrim commented Feb 24, 2025 •

edited

Loading

kostasrim Feb 24, 2025 •

edited

Loading

kostasrim commented Feb 24, 2025 •

edited

Loading

kostasrim Feb 26, 2025 •

edited

Loading

kostasrim Feb 26, 2025 •

edited

Loading

kostasrim Feb 26, 2025 •

edited

Loading

kostasrim Feb 26, 2025 •

edited

Loading