Skip to content

Commit a9056ec

Browse files
pan-applepan-apple
committed
address review comments
1 parent b55e808 commit a9056ec

File tree

4 files changed

+49
-60
lines changed

4 files changed

+49
-60
lines changed

‎src/messaging/ExchangeMgr.cpp

+10-14
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,12 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
207207
payloadHeader.GetMessageType(), payloadHeader.GetProtocolID().ToFullyQualifiedSpecForm(),
208208
payloadHeader.GetExchangeID());
209209

210+
MessageFlags msgFlags;
211+
if (isDuplicate == DuplicateMessage::Yes)
212+
{
213+
msgFlags.Set(MessageFlagValues::kDuplicateMessage);
214+
}
215+
210216
// Search for an existing exchange that the message applies to. If a match is found...
211217
bool found = false;
212218
mContextPool.ForEachActiveObject([&](auto * ec) {
@@ -219,11 +225,6 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
219225
ec->SetMsgRcvdFromPeer(true);
220226
}
221227

222-
MessageFlags msgFlags;
223-
if (isDuplicate == DuplicateMessage::kYes)
224-
{
225-
msgFlags.Set(MessageFlagValues::kDuplicateMessage);
226-
}
227228
// Matched ExchangeContext; send to message handler.
228229
ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf));
229230
found = true;
@@ -232,15 +233,15 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
232233
return true;
233234
});
234235

235-
if (found || isDuplicate == DuplicateMessage::kYes)
236+
if (found)
236237
{
237238
ExitNow(err = CHIP_NO_ERROR);
238239
}
239240

240-
// Search for an unsolicited message handler if it marked as being sent by an initiator. Since we didn't
241-
// find an existing exchange that matches the message, it must be an unsolicited message. However all
241+
// If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator.
242+
// Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all
242243
// unsolicited messages must be marked as being from an initiator.
243-
if (payloadHeader.IsInitiator())
244+
if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator())
244245
{
245246
// Search for an unsolicited message handler that can handle the message. Prefer handlers that can explicitly
246247
// handle the message type over handlers that handle all messages for a profile.
@@ -293,11 +294,6 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
293294

294295
ChipLogDetail(ExchangeManager, "ec id: %d, Delegate: 0x%p", ec->GetExchangeId(), ec->GetDelegate());
295296

296-
MessageFlags msgFlags;
297-
if (isDuplicate == DuplicateMessage::kYes)
298-
{
299-
msgFlags.Set(MessageFlagValues::kDuplicateMessage);
300-
}
301297
ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf));
302298

303299
// Close exchange if it was created only to send ack for a duplicate message.

‎src/messaging/tests/TestReliableMessageProtocol.cpp

+33-40
Original file line numberDiff line numberDiff line change
@@ -99,18 +99,19 @@ class MockAppDelegate : public ExchangeDelegate
9999
{
100100
ec->GetReliableMessageContext()->SetAckPending(false);
101101
}
102+
103+
if (mExchange != ec)
104+
{
105+
CloseExchangeIfNeeded();
106+
}
107+
102108
if (!mRetainExchange)
103109
{
104110
ec->Close();
105111
ec = nullptr;
106-
107-
if (mExchange != nullptr)
108-
{
109-
mExchange->Close();
110-
mExchange = nullptr;
111-
}
112112
}
113113
mExchange = ec;
114+
114115
if (mTestSuite != nullptr)
115116
{
116117
NL_TEST_ASSERT(mTestSuite, buffer->TotalLength() == sizeof(PAYLOAD));
@@ -121,6 +122,15 @@ class MockAppDelegate : public ExchangeDelegate
121122

122123
void OnResponseTimeout(ExchangeContext * ec) override {}
123124

125+
void CloseExchangeIfNeeded()
126+
{
127+
if (mExchange != nullptr)
128+
{
129+
mExchange->Close();
130+
mExchange = nullptr;
131+
}
132+
}
133+
124134
bool IsOnMessageReceivedCalled = false;
125135
bool mDropAckResponse = false;
126136
bool mRetainExchange = false;
@@ -494,7 +504,7 @@ void CheckResendApplicationMessageWithPeerExchange(nlTestSuite * inSuite, void *
494504
rm->ClearRetransTable(rc);
495505
}
496506

497-
void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inContext)
507+
void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext)
498508
{
499509
TestContext & ctx = *reinterpret_cast<TestContext *>(inContext);
500510

@@ -525,58 +535,44 @@ void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inC
525535
1, // CHIP_CONFIG_RMP_DEFAULT_ACTIVE_RETRY_INTERVAL
526536
});
527537

528-
mockReceiver.mDropAckResponse = true;
529-
530-
// Let's not drop any messages. We'll drop acks for this test.
538+
// Let's not drop the message. Expectation is that it is received by the peer, but the ack is dropped
531539
gLoopback.mSendMessageCount = 0;
532540
gLoopback.mNumMessagesToDrop = 0;
533541
gLoopback.mDroppedMessageCount = 0;
534542

543+
// Drop the ack, and also close the peer exchange
544+
mockReceiver.mDropAckResponse = true;
545+
mockReceiver.mRetainExchange = false;
546+
535547
// Ensure the retransmit table is empty right now
536548
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
537549

538550
err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer));
539551
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
540-
exchange->Close();
541552

542-
// Ensure the message was not dropped, and was added to retransmit table
553+
// Ensure the message was sent
554+
// The ack was dropped, and message was added to the retransmit table
543555
NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 1);
544556
NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0);
545557
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);
546-
NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled);
547558

548-
// 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit
549-
test_os_sleep_ms(65);
550-
ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR);
551-
552-
// Ensure the retransmit message was also not dropped, and is still there in the retransmit table
553-
NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 2);
554-
NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0);
555-
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);
556-
NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled);
557-
558-
// Let's not drop the ack on the next retry
559+
// Let's not drop the duplicate message
559560
mockReceiver.mDropAckResponse = false;
560561

561562
// 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit
562563
test_os_sleep_ms(65);
563564
ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR);
564565

565-
// Ensure the message was retransmitted, and is no longer in the retransmit table
566-
NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount >= 3);
566+
// Ensure the retransmit message was sent and the ack was sent
567+
// and retransmit table was cleared
568+
NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 3);
567569
NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0);
570+
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
568571

569-
// TODO - Enable test for lost CRMP ack messages
570-
// The following check is commented out because of https://github.com/project-chip/connectedhomeip/issues/7292
571-
// NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
572-
NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled);
573-
574-
mockReceiver.mTestSuite = nullptr;
575-
576-
err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest);
577-
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
572+
mockReceiver.CloseExchangeIfNeeded();
578573

579574
rm->ClearRetransTable(rc);
575+
exchange->Close();
580576
}
581577

582578
void CheckResendSessionEstablishmentMessageWithPeerExchange(nlTestSuite * inSuite, void * inContext)
@@ -733,10 +729,7 @@ void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext)
733729
NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0);
734730
NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
735731

736-
if (mockReceiver.mExchange != nullptr)
737-
{
738-
mockReceiver.mExchange->Close();
739-
}
732+
mockReceiver.CloseExchangeIfNeeded();
740733

741734
rm->ClearRetransTable(rc);
742735
exchange->Close();
@@ -776,9 +769,9 @@ const nlTest sTests[] =
776769
NL_TEST_DEF("Test ReliableMessageMgr::CheckCloseExchangeAndResendApplicationMessage", CheckCloseExchangeAndResendApplicationMessage),
777770
NL_TEST_DEF("Test ReliableMessageMgr::CheckFailedMessageRetainOnSend", CheckFailedMessageRetainOnSend),
778771
NL_TEST_DEF("Test ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange", CheckResendApplicationMessageWithPeerExchange),
779-
NL_TEST_DEF("Test ReliableMessageMgr::CheckResendApplicationMessageWithLostAcks", CheckResendApplicationMessageWithLostAcks),
780772
NL_TEST_DEF("Test ReliableMessageMgr::CheckResendSessionEstablishmentMessageWithPeerExchange", CheckResendSessionEstablishmentMessageWithPeerExchange),
781773
NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessage", CheckDuplicateMessage),
774+
NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessageClosedExchange", CheckDuplicateMessageClosedExchange),
782775
NL_TEST_DEF("Test ReliableMessageMgr::CheckSendStandaloneAckMessage", CheckSendStandaloneAckMessage),
783776

784777
NL_TEST_SENTINEL()

‎src/transport/SecureSessionMgr.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ void SecureSessionMgr::MessageDispatch(const PacketHeader & packetHeader, const
333333
PayloadHeader payloadHeader;
334334
ReturnOnFailure(payloadHeader.DecodeAndConsume(msg));
335335
mCB->OnMessageReceived(packetHeader, payloadHeader, SecureSessionHandle(), peerAddress,
336-
SecureSessionMgrDelegate::DuplicateMessage::kNo, std::move(msg));
336+
SecureSessionMgrDelegate::DuplicateMessage::No, std::move(msg));
337337
}
338338
}
339339

@@ -352,7 +352,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader,
352352
NodeId localNodeId;
353353
FabricId fabricId;
354354

355-
SecureSessionMgrDelegate::DuplicateMessage isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::kNo;
355+
SecureSessionMgrDelegate::DuplicateMessage isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::No;
356356

357357
VerifyOrExit(!msg.IsNull(), ChipLogError(Inet, "Secure transport received NULL packet, discarding"));
358358

@@ -395,7 +395,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader,
395395
if (err == CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED)
396396
{
397397
ChipLogDetail(Inet, "Received a duplicate message");
398-
isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::kYes;
398+
isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::Yes;
399399
err = CHIP_NO_ERROR;
400400
}
401401
if (err != CHIP_NO_ERROR)
@@ -437,7 +437,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader,
437437
VerifyOrExit(CHIP_NO_ERROR == SecureMessageCodec::Decode(state, payloadHeader, packetHeader, msg),
438438
ChipLogError(Inet, "Secure transport received message, but failed to decode it, discarding"));
439439

440-
if (isDuplicate == SecureSessionMgrDelegate::DuplicateMessage::kYes && !payloadHeader.NeedsAck())
440+
if (isDuplicate == SecureSessionMgrDelegate::DuplicateMessage::Yes && !payloadHeader.NeedsAck())
441441
{
442442
// If it's a duplicate message, but doesn't require an ack, let's drop it right here to save CPU
443443
// cycles on further message processing.

‎src/transport/SecureSessionMgr.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,8 @@ class DLL_EXPORT SecureSessionMgrDelegate
125125
public:
126126
enum class DuplicateMessage : uint8_t
127127
{
128-
kYes,
129-
kNo,
128+
Yes,
129+
No,
130130
};
131131

132132
/**

0 commit comments

Comments
 (0)