From c1624358c43c021f41907b9303e3cd4371529522 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 5 Oct 2023 14:16:25 -0400 Subject: [PATCH] address review comments --- .../ota-requestor/DefaultOTARequestor.h | 2 +- src/messaging/ExchangeContext.cpp | 13 ++++++----- src/messaging/ExchangeContext.h | 22 +++++++++---------- src/messaging/ReliableMessageContext.h | 4 ++-- src/messaging/ReliableMessageMgr.cpp | 4 ++-- src/messaging/ReliableMessageMgr.h | 4 ++-- .../tests/TestReliableMessageProtocol.cpp | 12 +++++----- src/protocols/bdx/TransferFacilitator.cpp | 2 +- src/protocols/secure_channel/CASESession.cpp | 6 ++--- src/protocols/secure_channel/PASESession.cpp | 2 +- 10 files changed, 37 insertions(+), 34 deletions(-) diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.h b/src/app/clusters/ota-requestor/DefaultOTARequestor.h index cdc34564414be7..fd430fe7b804d6 100644 --- a/src/app/clusters/ota-requestor/DefaultOTARequestor.h +++ b/src/app/clusters/ota-requestor/DefaultOTARequestor.h @@ -165,7 +165,7 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader:: if (!payloadHeader.HasMessageType(chip::Protocols::SecureChannel::MsgType::StatusReport)) { ec->WillSendMessage(); - ec->SetShouldPeerBeActive(true); + ec->SetPeerActiveStateHint(true); } return CHIP_NO_ERROR; diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index b223445ca5e982..4fef5b310fe9a7 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -537,9 +537,12 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload // layer has completed its work on the ExchangeContext. ExchangeHandle ref(*this); - // If we received a message from the Peer and we are still expecting a response, we know the peer is active - // If we receive a message from the Peer but we are not expecting a response, we don't know if the peer is still active - SetShouldPeerBeActive(IsResponseExpected()); + // If we receive a message while we are expecting a response for our previous message, + // we assume that the peer is likely active. If the message was in fact the response, + // the flag will be reset to false when we process the message later in the function. + // If we receive a message while we are not expecting a response, + // we reset the flag to false because we don't if the peer should still be active or not. + SetPeerActiveStateHint(IsResponseExpected()); bool isStandaloneAck = payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::StandaloneAck); bool isDuplicate = msgFlags.Has(MessageFlagValues::kDuplicateMessage); @@ -633,7 +636,7 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload SetResponseExpected(false); // If we received the expected response, we don't if the peer is still active after having sent the response - SetShouldPeerBeActive(false); + SetPeerActiveStateHint(false); } // Don't send messages on to our delegate if our dispatch does not allow @@ -695,7 +698,7 @@ void ExchangeContext::ExchangeSessionHolder::GrabExpiredSession(const SessionHan GrabUnchecked(session); } -bool ExchangeContext::ShouldPeerBeActive() +bool ExchangeContext::IsPeerLikelyActiveHint() { // If we are not the initiator, it means the peer sent the first message. // This means our peer is active if we are sending a message to them. diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index b294ace4ac65eb..8f6b8d14473c99 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -217,19 +217,19 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, /** * Notifies the exchange that our consummer is going to send a message where it knows the peer (receiver) should be active * - * This API is used to set the ShouldPeerBeActive flag. - * The ShouldPeerBeActive flag allows the consummer of the exchange to notify the exchange that the peer (receiver) + * This API is used to set the IsPeerLikelyActiveHint flag. + * The IsPeerLikelyActiveHint flag allows the consummer of the exchange to notify the exchange that the peer (receiver) * will be active for the next message the consummer will send. * This allows the consummer to notify the exchange on the state of the peer with the context of the messages being sent. * - * The ShouldPeerBeActive flag is only used when calculating the next retransmission time in the ReliableMessageMgr. - * If the ShouldPeerBeActive flag is true, we will used the Active Retransmission timeout when calculating the next - * retransmission time. If the ShouldPeerBeActive flag is false, we will rely on other information to determine which - * retransmission timeout should be used. See the ShouldPeerBeActive API for the alternative means to determine if the Peer + * The IsPeerLikelyActiveHint flag is only used when calculating the next retransmission time in the ReliableMessageMgr. + * If the IsPeerLikelyActiveHint flag is true, we will used the Active Retransmission timeout when calculating the next + * retransmission time. If the IsPeerLikelyActiveHint flag is false, we will rely on other information to determine which + * retransmission timeout should be used. See the IsPeerLikelyActiveHint API for the alternative means to determine if the Peer * (receiver) is active. * - * The first message of the exchange set as the initiator should never have the ShouldPeerBeActive flag to true. - * The API should only called after receiving a message from the peer and after calling the WillSendMessage API. + * The first message of the exchange set as the initiator should never have the IsPeerLikelyActiveHint flag to true. + * The API should only be called after receiving a message from the peer and after calling the WillSendMessage API. * The next message sent will then use the active retransmission time out. * The flag is reset to false each time we receive the response from our peer (receiver). * As such, the API needs to be called each time we send another message over the exchange. @@ -237,9 +237,9 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, * The API call is not mandatory for the communication to be successful. * If the call is not done, we will rely on information within the exchange context. * - * @param shouldPeerBeActive true if the peer should be active, false if we don't know or it should not be + * @param IsPeerLikelyActiveHint true if the peer should be active, false if we don't know or it should not be */ - void SetShouldPeerBeActive(bool shouldPeerBeActive) { mFlags.Set(Flags::kPeerShouldBeActive, shouldPeerBeActive); } + void SetPeerActiveStateHint(bool IsPeerLikelyActiveHint) { mFlags.Set(Flags::kPeerShouldBeActive, IsPeerLikelyActiveHint); } /** * Function checks with information available in the ExchangeContext if the peer should be active or not @@ -253,7 +253,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, * @return false We weren't able to determine if the peer is active. * We don't know in which state the peer is. */ - bool ShouldPeerBeActive(); + bool IsPeerLikelyActiveHint(); #if CONFIG_BUILD_FOR_HOST_UNIT_TEST SessionHolder & GetSessionHolder() { return mSession; } diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index 00262a766f9067..29b70e008cbd5b 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -180,13 +180,13 @@ class ReliableMessageContext * * If we are not initiator -> peer is active * If ephemeral context -> peer is active - * else we default to information provided by the consummer + * else we default to information provided by the client * * @return true Based on available information, Peer should be active * @return false We weren't able to determine if the peer is active. * We don't know in which state the peer is. */ - bool ShouldPeerBeActive(); + bool IsPeerLikelyActiveHint(); private: void HandleRcvdAck(uint32_t ackMessageCounter); diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 54047eb5a968a1..56cbef2e7b8c5c 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -465,11 +465,11 @@ CHIP_ERROR ReliableMessageMgr::MapSendError(CHIP_ERROR error, uint16_t exchangeI return error; } -void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry * entry) +void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry *& entry) { System::Clock::Timestamp baseTimeout = System::Clock::Milliseconds64(0); - if (entry->ec->ShouldPeerBeActive()) + if (entry->ec->IsPeerLikelyActiveHint()) { // If we know the peer is active with the exchangeContext, use the Active Retrans timeout baseTimeout = entry->ec->GetSessionHandle()->GetRemoteMRPConfig().mActiveRetransTimeout; diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index 9191cd2ef1296d..e8c22efad6c307 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -210,9 +210,9 @@ class ReliableMessageMgr * Calculates the next retransmission time for the entry * Function sets the nextRetransTime of the entry * - * @param[in/out] entry RetransTableEntry for which we need to calculate the nextRetransTime + * @param[in,out] entry RetransTableEntry for which we need to calculate the nextRetransTime */ - void CalculateNextRetransTime(RetransTableEntry * entry); + void CalculateNextRetransTime(RetransTableEntry *& entry); ObjectPool & mContextPool; chip::System::Layer * mSystemLayer; diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 2c1edb32502dec..adab185d7f66c9 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -1572,7 +1572,7 @@ void TestReliableMessageProtocol::CheckIsPeerActiveNotInitiator(nlTestSuite * in /** * This tests the following scenario: * 1) A reliable message expecting a response is sent from the initiator to responder which is losts - * 2) Initiator resends the message a the IdleRetrans interval + * 2) Initiator resends the message at the IdleRetrans interval * 3) Responder receives the message and sends a standalone ack * 4) Responder sends a response and fails * 5) Responder retries at the ActiveRestrans interval @@ -1617,7 +1617,7 @@ void TestReliableMessageProtocol::CheckIsPeerActiveNotInitiator(nlTestSuite * in mockReceiver.mRetainExchange = true; mockSender.mRetainExchange = true; - NL_TEST_ASSERT(inSuite, !exchange->ShouldPeerBeActive()); + NL_TEST_ASSERT(inSuite, !exchange->IsPeerLikelyActiveHint()); err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer), SendFlags(SendMessageFlags::kExpectResponse)); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -1632,7 +1632,7 @@ void TestReliableMessageProtocol::CheckIsPeerActiveNotInitiator(nlTestSuite * in ctx.GetIOContext().DriveIOUntil(500_ms32, [&] { return loopback.mSentMessageCount >= 1; }); ctx.DrainAndServiceIO(); - NL_TEST_ASSERT(inSuite, !exchange->ShouldPeerBeActive()); + NL_TEST_ASSERT(inSuite, !exchange->IsPeerLikelyActiveHint()); // // Make sure nothing happened NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 1); @@ -1642,15 +1642,15 @@ void TestReliableMessageProtocol::CheckIsPeerActiveNotInitiator(nlTestSuite * in ctx.GetIOContext().DriveIOUntil(2000_ms32, [&] { return loopback.mSentMessageCount >= 2; }); ctx.DrainAndServiceIO(); - NL_TEST_ASSERT(inSuite, !exchange->ShouldPeerBeActive()); + NL_TEST_ASSERT(inSuite, !exchange->IsPeerLikelyActiveHint()); // // Make sure nothing happened NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 2); NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); // // Verify that the receiver considers the sender is active - NL_TEST_ASSERT(inSuite, !exchange->ShouldPeerBeActive()); - NL_TEST_ASSERT(inSuite, mockReceiver.mExchange->ShouldPeerBeActive()); + NL_TEST_ASSERT(inSuite, !exchange->IsPeerLikelyActiveHint()); + NL_TEST_ASSERT(inSuite, mockReceiver.mExchange->IsPeerLikelyActiveHint()); mockReceiver.mExchange->GetSessionHandle()->AsSecureSession()->SetRemoteMRPConfig({ 1000_ms32, // CHIP_CONFIG_MRP_LOCAL_IDLE_RETRY_INTERVAL diff --git a/src/protocols/bdx/TransferFacilitator.cpp b/src/protocols/bdx/TransferFacilitator.cpp index 19d5ca051687aa..cfc7ffc233dbdc 100644 --- a/src/protocols/bdx/TransferFacilitator.cpp +++ b/src/protocols/bdx/TransferFacilitator.cpp @@ -54,7 +54,7 @@ CHIP_ERROR TransferFacilitator::OnMessageReceived(chip::Messaging::ExchangeConte // For this reason, it is left up to the application logic to call ExchangeContext::Close() when it has determined that the // transfer is finished. mExchangeCtx->WillSendMessage(); - mExchangeCtx->SetShouldPeerBeActive(true); + mExchangeCtx->SetPeerActiveStateHint(true); return err; } diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 1152620e043a08..e97a309da7d056 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -1410,8 +1410,8 @@ CHIP_ERROR CASESession::SendSigma3a() SuccessOrExit(err = helper->ScheduleWork()); mSendSigma3Helper = helper; mExchangeCtxt->WillSendMessage(); - mExchangeCtxt->SetShouldPeerBeActive( - true); // When we send sigma3, peer has processed sigma 1 and sent sigma2 which means it is active + mExchangeCtxt->SetPeerActiveStateHint( + true); // When we send sigma3, peer has processed sigma 1 and sent sigma2 which means it is likely active mState = State::kSendSigma3Pending; } else @@ -1705,7 +1705,7 @@ CHIP_ERROR CASESession::HandleSigma3a(System::PacketBufferHandle && msg) SuccessOrExit(err = helper->ScheduleWork()); mHandleSigma3Helper = helper; mExchangeCtxt->WillSendMessage(); - mExchangeCtxt->SetShouldPeerBeActive( + mExchangeCtxt->SetPeerActiveStateHint( true); // When we receive sigma3, peer has received and processed sigma2 which means it is active mState = State::kHandleSigma3Pending; } diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 5a0e004f0d6114..0e3acbd8138336 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -826,7 +826,7 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl SuccessOrExit(err); // Once we receive a message for the PASE protocol, we know the Peer is active - mExchangeCtxt->SetShouldPeerBeActive(true); + mExchangeCtxt->SetPeerActiveStateHint(true); #if CHIP_CONFIG_SLOW_CRYPTO if (msgType == MsgType::PBKDFParamRequest || msgType == MsgType::PBKDFParamResponse || msgType == MsgType::PASE_Pake1 ||