Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@pan-apple
pan-apple committed Jun 26, 2021
1 parent fbf6c50 commit b9a8612
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 60 deletions.
24 changes: 10 additions & 14 deletions src/messaging/ExchangeMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
73 changes: 33 additions & 40 deletions src/messaging/tests/TestReliableMessageProtocol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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;
Expand Down Expand Up @@ -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<TestContext *>(inContext);

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
Expand Down
8 changes: 4 additions & 4 deletions src/transport/SecureSessionMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

Expand All @@ -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"));

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions src/transport/SecureSessionMgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ class DLL_EXPORT SecureSessionMgrDelegate
public:
enum class DuplicateMessage : uint8_t
{
kYes,
kNo,
Yes,
No,
};

/**
Expand Down

0 comments on commit b9a8612

Please sign in to comment.