From c6cad50417cfd4ceb0c9dfc53ce4ca4f9eb30147 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 7 Jul 2023 17:27:44 -0400 Subject: [PATCH] Fix TestReliableMessageProtocol timing bug. (#27817) --- .../tests/TestReliableMessageProtocol.cpp | 96 +++++++++++-------- 1 file changed, 56 insertions(+), 40 deletions(-) diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 101367e7f10428..7596071f07af8a 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -69,6 +69,8 @@ constexpr auto retryBoosterTimeout = CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS * CHIP_ class MockAppDelegate : public UnsolicitedMessageHandler, public ExchangeDelegate { public: + MockAppDelegate(TestContext & ctx) : mTestContext(ctx) {} + CHIP_ERROR OnUnsolicitedMessageReceived(const PayloadHeader & payloadHeader, ExchangeDelegate *& newDelegate) override { // Handle messages by myself @@ -89,6 +91,9 @@ class MockAppDelegate : public UnsolicitedMessageHandler, public ExchangeDelegat auto * rc = ec->GetReliableMessageContext(); if (rc->HasPiggybackAckPending()) { + // Make sure we don't accidentally retransmit and end up acking + // the retransmit. + rc->GetReliableMessageMgr()->StopTimer(); (void) rc->TakePendingPeerAckMessageCounter(); } } @@ -127,12 +132,27 @@ class MockAppDelegate : public UnsolicitedMessageHandler, public ExchangeDelegat } } + void SetDropAckResponse(bool dropResponse) + { + mDropAckResponse = dropResponse; + if (!mDropAckResponse) + { + // Restart the MRP retransmit timer, now that we are not going to be + // dropping acks anymore, so we send out pending retransmits, if + // any, as needed. + mTestContext.GetExchangeManager().GetReliableMessageMgr()->StartTimer(); + } + } + bool IsOnMessageReceivedCalled = false; bool mReceivedPiggybackAck = false; - bool mDropAckResponse = false; bool mRetainExchange = false; ExchangeContext * mExchange = nullptr; nlTestSuite * mTestSuite = nullptr; + +private: + TestContext & mTestContext; + bool mDropAckResponse = false; }; class MockSessionEstablishmentExchangeDispatch : public Messaging::ApplicationExchangeDispatch @@ -279,7 +299,7 @@ void CheckAddClearRetrans(nlTestSuite * inSuite, void * inContext) { TestContext & ctx = *reinterpret_cast(inContext); - MockAppDelegate mockAppDelegate; + MockAppDelegate mockAppDelegate(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockAppDelegate); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -337,7 +357,7 @@ void CheckResendApplicationMessage(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); // TODO: temporarily create a SessionHandle from node id, will be fix in PR 3602 ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -450,7 +470,7 @@ void CheckCloseExchangeAndResendApplicationMessage(nlTestSuite * inSuite, void * CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); // TODO: temporarily create a SessionHandle from node id, will be fixed in PR 3602 ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -594,13 +614,13 @@ void CheckResendApplicationMessageWithPeerExchange(nlTestSuite * inSuite, void * CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -656,13 +676,13 @@ void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -681,8 +701,8 @@ void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext loopback.mDroppedMessageCount = 0; // Drop the ack, and also close the peer exchange - mockReceiver.mDropAckResponse = true; - mockReceiver.mRetainExchange = false; + mockReceiver.SetDropAckResponse(true); + mockReceiver.mRetainExchange = false; // Ensure the retransmit table is empty right now NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); @@ -698,7 +718,7 @@ void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); // Let's not drop the duplicate message - mockReceiver.mDropAckResponse = false; + mockReceiver.SetDropAckResponse(false); err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -723,13 +743,13 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -748,8 +768,8 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont loopback.mDroppedMessageCount = 0; // Drop the ack, and also close the peer exchange - mockReceiver.mDropAckResponse = true; - mockReceiver.mRetainExchange = false; + mockReceiver.SetDropAckResponse(true); + mockReceiver.mRetainExchange = false; // Ensure the retransmit table is empty right now NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); @@ -764,10 +784,6 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 0); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - // Make sure we don't accidentally retransmit before we are done with our - // message counter incrementing. - rm->StopTimer(); - // Now send CHIP_CONFIG_MESSAGE_COUNTER_WINDOW_SIZE + 2 messages to make // sure our original message is out of the message counter window. These // messages can be sent withour MRP, because we are not expecting acks for @@ -798,7 +814,7 @@ void CheckDuplicateOldMessageClosedExchange(nlTestSuite * inSuite, void * inCont } // Let's not drop the duplicate message's ack. - mockReceiver.mDropAckResponse = false; + mockReceiver.SetDropAckResponse(false); err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -892,13 +908,13 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -917,8 +933,8 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext) loopback.mDroppedMessageCount = 0; // Drop the ack, and keep the exchange around to receive the duplicate message - mockReceiver.mDropAckResponse = true; - mockReceiver.mRetainExchange = true; + mockReceiver.SetDropAckResponse(true); + mockReceiver.mRetainExchange = true; // Ensure the retransmit table is empty right now NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); @@ -937,8 +953,8 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); // Let's not drop the duplicate message - mockReceiver.mDropAckResponse = false; - mockReceiver.mRetainExchange = false; + mockReceiver.SetDropAckResponse(false); + mockReceiver.mRetainExchange = false; // Wait for the first re-transmit and ack (should take 64ms) ctx.GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 3; }); @@ -962,13 +978,13 @@ void CheckReceiveAfterStandaloneAck(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1052,13 +1068,13 @@ void CheckPiggybackAfterPiggyback(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1191,7 +1207,7 @@ void CheckSendUnsolicitedStandaloneAckMessage(nlTestSuite * inSuite, void * inCo CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1229,7 +1245,7 @@ void CheckSendStandaloneAckMessage(nlTestSuite * inSuite, void * inContext) { TestContext & ctx = *reinterpret_cast(inContext); - MockAppDelegate mockAppDelegate; + MockAppDelegate mockAppDelegate(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockAppDelegate); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1268,13 +1284,13 @@ void CheckMessageAfterClosed(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1388,13 +1404,13 @@ void CheckLostResponseWithPiggyback(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1543,13 +1559,13 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = CHIP_NO_ERROR; - MockAppDelegate mockReceiver; + MockAppDelegate mockReceiver(ctx); err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); mockReceiver.mTestSuite = inSuite; - MockAppDelegate mockSender; + MockAppDelegate mockSender(ctx); ExchangeContext * exchange = ctx.NewExchangeToAlice(&mockSender); NL_TEST_ASSERT(inSuite, exchange != nullptr); @@ -1574,7 +1590,7 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext) mockSender.mRetainExchange = true; // And ensure the ack heading back our way is dropped. - mockReceiver.mDropAckResponse = true; + mockReceiver.SetDropAckResponse(true); err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer), SendFlags(SendMessageFlags::kExpectResponse)); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -1596,7 +1612,7 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, !receiverRc->IsAckPending()); // Don't drop any more acks. - mockReceiver.mDropAckResponse = false; + mockReceiver.SetDropAckResponse(false); // Now send a message from the other side. buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD));