From 05e93dadc080e45d624d92b80879297cfade417c Mon Sep 17 00:00:00 2001 From: Austin Foxley Date: Fri, 4 Oct 2024 21:02:00 +0000 Subject: [PATCH] pw_rpc: Fix Call not getting reset on default constructor assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When trying to reinitialize an already used Call object to default, the move constructor runs but ends up exiting early before actually resetting the call internal state due to this conditional: if (!other.active_locked()) { return; // Nothing else to do; this call is already closed. } A default Call has state_ initialized to 0, so active_locked() returns false so none of the assignments happen on `this`. We fix this by letting the bulk of the move assignments happen regardless of active state, except for those parts that apply specifically to active calls. Bug: 371211198 Change-Id: I8cb18d06bd91db41822937a57b31351049678222 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/239718 Docs-Not-Needed: Austin Foxley Commit-Queue: Austin Foxley Reviewed-by: Wyatt Hepler Lint: Lint 🤖 --- pw_rpc/call.cc | 17 +++++++---------- pw_rpc/call_test.cc | 8 ++++++++ .../pw_rpc_private/fake_server_reader_writer.h | 2 ++ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/pw_rpc/call.cc b/pw_rpc/call.cc index 976c781b53..859853073b 100644 --- a/pw_rpc/call.cc +++ b/pw_rpc/call.cc @@ -144,13 +144,9 @@ void Call::MoveFrom(Call& other) { PW_DCHECK(!active_locked()); PW_DCHECK(!awaiting_cleanup() && !other.awaiting_cleanup()); - if (!other.active_locked()) { - return; // Nothing else to do; this call is already closed. - } - // An active call with an executing callback cannot be moved. Derived call // classes must wait for callbacks to finish before calling MoveFrom. - PW_DCHECK(!other.CallbacksAreRunning()); + PW_DCHECK(!other.active_locked() || !other.CallbacksAreRunning()); // Copy all members from the other call. endpoint_ = other.endpoint_; @@ -171,11 +167,12 @@ void Call::MoveFrom(Call& other) { on_error_ = std::move(other.on_error_); on_next_ = std::move(other.on_next_); - // Mark the other call inactive, unregister it, and register this one. - other.MarkClosed(); - - endpoint().UnregisterCall(other); - endpoint().RegisterUniqueCall(*this); + if (other.active_locked()) { + // Mark the other call inactive, unregister it, and register this one. + other.MarkClosed(); + endpoint().UnregisterCall(other); + endpoint().RegisterUniqueCall(*this); + } } void Call::WaitUntilReadyForMove(Call& destination, Call& source) { diff --git a/pw_rpc/call_test.cc b/pw_rpc/call_test.cc index dac93550d4..4dbc167567 100644 --- a/pw_rpc/call_test.cc +++ b/pw_rpc/call_test.cc @@ -337,6 +337,14 @@ TEST_F(ServerReaderWriterTest, Move_ClearsCallAndChannelId) { EXPECT_EQ(reader_writer_.channel_id_locked(), 0u); } +TEST_F(ServerReaderWriterTest, DefaultConstructorAssign_Reset) { + reader_writer_ = {}; + + RpcLockGuard lock; + EXPECT_EQ(reader_writer_.service_id(), 0u); + EXPECT_EQ(reader_writer_.method_id(), 0u); +} + TEST_F(ServerReaderWriterTest, Move_SourceAwaitingCleanup_CleansUpCalls) { std::optional on_error_cb; reader_writer_.set_on_error([&on_error_cb](Status error) { diff --git a/pw_rpc/pw_rpc_private/fake_server_reader_writer.h b/pw_rpc/pw_rpc_private/fake_server_reader_writer.h index ef24c126e2..f9f36b825f 100644 --- a/pw_rpc/pw_rpc_private/fake_server_reader_writer.h +++ b/pw_rpc/pw_rpc_private/fake_server_reader_writer.h @@ -55,6 +55,8 @@ class FakeServerReaderWriter : private ServerCall { // `PW_LOCKS_EXCLUDED(rpc_lock())` on their original definitions does not // appear to carry through here. using Call::active; + using Call::method_id; + using Call::service_id; using Call::set_on_error; using Call::set_on_next; using ServerCall::set_on_completion_requested;