pw_rpc: Replace existing client calls in RegisterCall
This updates the RPC client to replace (cancel) an existing call when a
new one is started for the same RPC instead of failing. This is
consistent with the Python client and expected user behavior (calling an
RPC with the same context should replace the first).
Change-Id: Idb5096a19b83b291cbb714204bc11be5eaa47947
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/60820
Pigweed-Auto-Submit: Alexei Frolov <frolv@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
diff --git a/pw_rpc/base_client_call.cc b/pw_rpc/base_client_call.cc
index 4c654e9..9ef3a31 100644
--- a/pw_rpc/base_client_call.cc
+++ b/pw_rpc/base_client_call.cc
@@ -35,11 +35,7 @@
// If the call being assigned is active, replace it in the client's list
// with a reference to the current object.
other.Unregister();
-
- // RegisterCall() only fails if there is already a call for the same
- // (channel, service, method). As the existing call is unregistered, this
- // error cannot happen.
- other.client_->RegisterCall(*this).IgnoreError();
+ other.client_->RegisterCall(*this);
}
return *this;
@@ -92,12 +88,7 @@
return Packet(type, channel_id_, service_id_, method_id_, payload);
}
-void BaseClientCall::Register() {
- // TODO(frolv): This is broken. If you try to replace an exisitng call with a
- // new call to the same method on the same channel, the new call will fail to
- // register. Instead, the new call should replace the existing one.
- client_->RegisterCall(*this).IgnoreError();
-}
+void BaseClientCall::Register() { client_->RegisterCall(*this); }
void BaseClientCall::Unregister() {
if (active()) {
diff --git a/pw_rpc/base_client_call_test.cc b/pw_rpc/base_client_call_test.cc
index 7583966..0f78d37 100644
--- a/pw_rpc/base_client_call_test.cc
+++ b/pw_rpc/base_client_call_test.cc
@@ -36,6 +36,28 @@
EXPECT_EQ(context.client().active_calls(), 0u);
}
+TEST(BaseClientCall, Register_ReplacesExistingCall) {
+ ClientContextForTest context;
+ EXPECT_EQ(context.client().active_calls(), 0u);
+
+ BaseClientCall first(&context.client(),
+ context.channel().id(),
+ context.service_id(),
+ context.method_id(),
+ [](BaseClientCall&, const Packet&) {});
+ EXPECT_TRUE(first.active());
+ EXPECT_EQ(context.client().active_calls(), 1u);
+
+ BaseClientCall second(&context.client(),
+ context.channel().id(),
+ context.service_id(),
+ context.method_id(),
+ [](BaseClientCall&, const Packet&) {});
+ EXPECT_TRUE(second.active());
+ EXPECT_EQ(context.client().active_calls(), 1u);
+ EXPECT_FALSE(first.active());
+}
+
TEST(BaseClientCall, Move_UnregistersOriginal) {
ClientContextForTest context;
EXPECT_EQ(context.client().active_calls(), 0u);
diff --git a/pw_rpc/client.cc b/pw_rpc/client.cc
index 3679e33..5b8b78f 100644
--- a/pw_rpc/client.cc
+++ b/pw_rpc/client.cc
@@ -98,20 +98,24 @@
return channel;
}
-Status Client::RegisterCall(BaseClientCall& call) {
+void Client::RegisterCall(BaseClientCall& call) {
auto existing_call = std::find_if(calls_.begin(), calls_.end(), [&](auto& c) {
return c.channel_id() == call.channel_id() &&
c.service_id() == call.service_id() &&
c.method_id() == call.method_id();
});
+
if (existing_call != calls_.end()) {
- PW_LOG_WARN(
- "RPC client tried to call same method multiple times; aborting.");
- return Status::FailedPrecondition();
+ PW_LOG_DEBUG(
+ "RPC client called same method multiple times; canceling existing "
+ "call.");
+
+ // TODO(frolv): Invoke the existing_call's error callback once client calls
+ // are refactored as generic Calls.
+ existing_call->Unregister();
}
calls_.push_front(call);
- return OkStatus();
}
} // namespace pw::rpc
diff --git a/pw_rpc/public/pw_rpc/client.h b/pw_rpc/public/pw_rpc/client.h
index cac5825..be3a2f2 100644
--- a/pw_rpc/public/pw_rpc/client.h
+++ b/pw_rpc/public/pw_rpc/client.h
@@ -54,7 +54,7 @@
private:
friend class internal::BaseClientCall;
- Status RegisterCall(internal::BaseClientCall& call);
+ void RegisterCall(internal::BaseClientCall& call);
void RemoveCall(const internal::BaseClientCall& call) { calls_.remove(call); }
std::span<internal::Channel> channels_;
