From b9a86128da36042a5297fd7a6fdfe78b34f0e7d6 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Fri, 25 Jun 2021 15:22:35 -0700 Subject: [PATCH] address review comments --- src/messaging/ExchangeMgr.cpp | 24 +++--- .../tests/TestReliableMessageProtocol.cpp | 73 +++++++++---------- src/transport/SecureSessionMgr.cpp | 8 +- src/transport/SecureSessionMgr.h | 4 +- 4 files changed, 49 insertions(+), 60 deletions(-) diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 22162a7ff16875..a769574354bbd4 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -207,6 +207,12 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const payloadHeader.GetMessageType(), payloadHeader.GetProtocolID().ToFullyQualifiedSpecForm(), payloadHeader.GetExchangeID()); + MessageFlags msgFlags; + if (isDuplicate == DuplicateMessage::Yes) + { + msgFlags.Set(MessageFlagValues::kDuplicateMessage); + } + // Search for an existing exchange that the message applies to. If a match is found... bool found = false; mContextPool.ForEachActiveObject([&](auto * ec) { @@ -219,11 +225,6 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const ec->SetMsgRcvdFromPeer(true); } - MessageFlags msgFlags; - if (isDuplicate == DuplicateMessage::kYes) - { - msgFlags.Set(MessageFlagValues::kDuplicateMessage); - } // Matched ExchangeContext; send to message handler. ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf)); found = true; @@ -232,15 +233,15 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const return true; }); - if (found || isDuplicate == DuplicateMessage::kYes) + if (found) { ExitNow(err = CHIP_NO_ERROR); } - // Search for an unsolicited message handler if it marked as being sent by an initiator. Since we didn't - // find an existing exchange that matches the message, it must be an unsolicited message. However all + // If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator. + // Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all // unsolicited messages must be marked as being from an initiator. - if (payloadHeader.IsInitiator()) + if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator()) { // Search for an unsolicited message handler that can handle the message. Prefer handlers that can explicitly // handle the message type over handlers that handle all messages for a profile. @@ -293,11 +294,6 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const ChipLogDetail(ExchangeManager, "ec id: %d, Delegate: 0x%p", ec->GetExchangeId(), ec->GetDelegate()); - MessageFlags msgFlags; - if (isDuplicate == DuplicateMessage::kYes) - { - msgFlags.Set(MessageFlagValues::kDuplicateMessage); - } ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf)); // Close exchange if it was created only to send ack for a duplicate message. diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index e5ae837becc093..078ce532c27cb0 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -99,18 +99,19 @@ class MockAppDelegate : public ExchangeDelegate { ec->GetReliableMessageContext()->SetAckPending(false); } + + if (mExchange != ec) + { + CloseExchangeIfNeeded(); + } + if (!mRetainExchange) { ec->Close(); ec = nullptr; - - if (mExchange != nullptr) - { - mExchange->Close(); - mExchange = nullptr; - } } mExchange = ec; + if (mTestSuite != nullptr) { NL_TEST_ASSERT(mTestSuite, buffer->TotalLength() == sizeof(PAYLOAD)); @@ -121,6 +122,15 @@ class MockAppDelegate : public ExchangeDelegate void OnResponseTimeout(ExchangeContext * ec) override {} + void CloseExchangeIfNeeded() + { + if (mExchange != nullptr) + { + mExchange->Close(); + mExchange = nullptr; + } + } + bool IsOnMessageReceivedCalled = false; bool mDropAckResponse = false; bool mRetainExchange = false; @@ -494,7 +504,7 @@ void CheckResendApplicationMessageWithPeerExchange(nlTestSuite * inSuite, void * rm->ClearRetransTable(rc); } -void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inContext) +void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext) { TestContext & ctx = *reinterpret_cast(inContext); @@ -525,58 +535,44 @@ void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inC 1, // CHIP_CONFIG_RMP_DEFAULT_ACTIVE_RETRY_INTERVAL }); - mockReceiver.mDropAckResponse = true; - - // Let's not drop any messages. We'll drop acks for this test. + // Let's not drop the message. Expectation is that it is received by the peer, but the ack is dropped gLoopback.mSendMessageCount = 0; gLoopback.mNumMessagesToDrop = 0; gLoopback.mDroppedMessageCount = 0; + // Drop the ack, and also close the peer exchange + mockReceiver.mDropAckResponse = true; + mockReceiver.mRetainExchange = false; + // Ensure the retransmit table is empty right now NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer)); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - exchange->Close(); - // Ensure the message was not dropped, and was added to retransmit table + // Ensure the message was sent + // The ack was dropped, and message was added to the retransmit table NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 1); NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); - // 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit - test_os_sleep_ms(65); - ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR); - - // Ensure the retransmit message was also not dropped, and is still there in the retransmit table - NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 2); - NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); - NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); - - // Let's not drop the ack on the next retry + // Let's not drop the duplicate message mockReceiver.mDropAckResponse = false; // 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit test_os_sleep_ms(65); ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR); - // Ensure the message was retransmitted, and is no longer in the retransmit table - NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount >= 3); + // Ensure the retransmit message was sent and the ack was sent + // and retransmit table was cleared + NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 3); NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); - // TODO - Enable test for lost CRMP ack messages - // The following check is commented out because of https://github.com/project-chip/connectedhomeip/issues/7292 - // NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); - NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); - - mockReceiver.mTestSuite = nullptr; - - err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + mockReceiver.CloseExchangeIfNeeded(); rm->ClearRetransTable(rc); + exchange->Close(); } void CheckResendSessionEstablishmentMessageWithPeerExchange(nlTestSuite * inSuite, void * inContext) @@ -733,10 +729,7 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); - if (mockReceiver.mExchange != nullptr) - { - mockReceiver.mExchange->Close(); - } + mockReceiver.CloseExchangeIfNeeded(); rm->ClearRetransTable(rc); exchange->Close(); @@ -776,9 +769,9 @@ const nlTest sTests[] = NL_TEST_DEF("Test ReliableMessageMgr::CheckCloseExchangeAndResendApplicationMessage", CheckCloseExchangeAndResendApplicationMessage), NL_TEST_DEF("Test ReliableMessageMgr::CheckFailedMessageRetainOnSend", CheckFailedMessageRetainOnSend), NL_TEST_DEF("Test ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange", CheckResendApplicationMessageWithPeerExchange), - NL_TEST_DEF("Test ReliableMessageMgr::CheckResendApplicationMessageWithLostAcks", CheckResendApplicationMessageWithLostAcks), NL_TEST_DEF("Test ReliableMessageMgr::CheckResendSessionEstablishmentMessageWithPeerExchange", CheckResendSessionEstablishmentMessageWithPeerExchange), NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessage", CheckDuplicateMessage), + NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessageClosedExchange", CheckDuplicateMessageClosedExchange), NL_TEST_DEF("Test ReliableMessageMgr::CheckSendStandaloneAckMessage", CheckSendStandaloneAckMessage), NL_TEST_SENTINEL() diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index 2b45cdd1341aa0..6d981c46717c69 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -333,7 +333,7 @@ void SecureSessionMgr::MessageDispatch(const PacketHeader & packetHeader, const PayloadHeader payloadHeader; ReturnOnFailure(payloadHeader.DecodeAndConsume(msg)); mCB->OnMessageReceived(packetHeader, payloadHeader, SecureSessionHandle(), peerAddress, - SecureSessionMgrDelegate::DuplicateMessage::kNo, std::move(msg)); + SecureSessionMgrDelegate::DuplicateMessage::No, std::move(msg)); } } @@ -352,7 +352,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, NodeId localNodeId; FabricId fabricId; - SecureSessionMgrDelegate::DuplicateMessage isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::kNo; + SecureSessionMgrDelegate::DuplicateMessage isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::No; VerifyOrExit(!msg.IsNull(), ChipLogError(Inet, "Secure transport received NULL packet, discarding")); @@ -395,7 +395,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, if (err == CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED) { ChipLogDetail(Inet, "Received a duplicate message"); - isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::kYes; + isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::Yes; err = CHIP_NO_ERROR; } if (err != CHIP_NO_ERROR) @@ -437,7 +437,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, VerifyOrExit(CHIP_NO_ERROR == SecureMessageCodec::Decode(state, payloadHeader, packetHeader, msg), ChipLogError(Inet, "Secure transport received message, but failed to decode it, discarding")); - if (isDuplicate == SecureSessionMgrDelegate::DuplicateMessage::kYes && !payloadHeader.NeedsAck()) + if (isDuplicate == SecureSessionMgrDelegate::DuplicateMessage::Yes && !payloadHeader.NeedsAck()) { // If it's a duplicate message, but doesn't require an ack, let's drop it right here to save CPU // cycles on further message processing. diff --git a/src/transport/SecureSessionMgr.h b/src/transport/SecureSessionMgr.h index 2fdbdf9b0e5e82..ca0ff7e069a0e8 100644 --- a/src/transport/SecureSessionMgr.h +++ b/src/transport/SecureSessionMgr.h @@ -125,8 +125,8 @@ class DLL_EXPORT SecureSessionMgrDelegate public: enum class DuplicateMessage : uint8_t { - kYes, - kNo, + Yes, + No, }; /**