diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index c9d912daf88091..31d579e660502c 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -113,7 +113,7 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp // If session requires MRP, NoAutoRequestAck send flag is not specified and is not a group exchange context, request reliable // transmission. bool reliableTransmissionRequested = - GetSessionHandle()->RequireMRP() && !sendFlags.Has(SendMessageFlags::kNoAutoRequestAck) && !IsGroupExchangeContext(); + GetSessionHandle()->AllowsMRP() && !sendFlags.Has(SendMessageFlags::kNoAutoRequestAck) && !IsGroupExchangeContext(); bool currentMessageExpectResponse = false; // If a response message is expected... @@ -322,8 +322,8 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons SetAckPending(false); - // Do not request Ack for multicast - SetAutoRequestAck(!session->IsGroupSession()); + // Try to use MRP by default, if it is allowed. + SetAutoRequestAck(session->AllowsMRP()); #if CHIP_CONFIG_ENABLE_ICD_SERVER app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlag::kExchangeContextOpen); @@ -531,34 +531,37 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload MessageHandled(); }); - if (mDispatch.IsReliableTransmissionAllowed() && !IsGroupExchangeContext()) + if (mSession->AllowsMRP()) { - if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() && - payloadHeader.GetAckMessageCounter().HasValue()) + if (mDispatch.IsReliableTransmissionAllowed()) { - HandleRcvdAck(payloadHeader.GetAckMessageCounter().Value()); + if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() && + payloadHeader.GetAckMessageCounter().HasValue()) + { + HandleRcvdAck(payloadHeader.GetAckMessageCounter().Value()); + } + + if (payloadHeader.NeedsAck()) + { + // An acknowledgment needs to be sent back to the peer for this message on this exchange, + HandleNeedsAck(messageCounter, msgFlags); + } } - if (payloadHeader.NeedsAck()) + if (IsAckPending() && !mDelegate) { - // An acknowledgment needs to be sent back to the peer for this message on this exchange, - HandleNeedsAck(messageCounter, msgFlags); + // The incoming message wants an ack, but we have no delegate, so + // there's not going to be a response to piggyback on. Just flush the + // ack out right now. + ReturnErrorOnFailure(FlushAcks()); } - } - if (IsAckPending() && !mDelegate) - { - // The incoming message wants an ack, but we have no delegate, so - // there's not going to be a response to piggyback on. Just flush the - // ack out right now. - ReturnErrorOnFailure(FlushAcks()); - } - - // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer. - if (isStandaloneAck) - { - return CHIP_NO_ERROR; - } + // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer. + if (isStandaloneAck) + { + return CHIP_NO_ERROR; + } + } // AllowsMRP // Since the message is duplicate, let's not forward it up the stack if (isDuplicate) @@ -566,23 +569,26 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload return CHIP_NO_ERROR; } - if (IsEphemeralExchange()) + if (mSession->AllowsMRP()) { - // The EphemeralExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. - return CHIP_NO_ERROR; - } + if (IsEphemeralExchange()) + { + // The EphemeralExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. + return CHIP_NO_ERROR; + } - if (IsWaitingForAck()) - { - // The only way we can get here is a spec violation on the other side: - // we sent a message that needs an ack, and the other side responded - // with a message that does not contain an ack for the message we sent. - // Just drop this message; if we delivered it to our delegate it might - // try to send another message-needing-an-ack in response, which would - // violate our internal invariants. - ChipLogError(ExchangeManager, "Dropping message without piggyback ack when we are waiting for an ack."); - return CHIP_ERROR_INCORRECT_STATE; - } + if (IsWaitingForAck()) + { + // The only way we can get here is a spec violation on the other side: + // we sent a message that needs an ack, and the other side responded + // with a message that does not contain an ack for the message we sent. + // Just drop this message; if we delivered it to our delegate it might + // try to send another message-needing-an-ack in response, which would + // violate our internal invariants. + ChipLogError(ExchangeManager, "Dropping message without piggyback ack when we are waiting for an ack."); + return CHIP_ERROR_INCORRECT_STATE; + } + } // AllowsMRP #if CHIP_CONFIG_ENABLE_ICD_SERVER // message received diff --git a/src/messaging/ExchangeMessageDispatch.cpp b/src/messaging/ExchangeMessageDispatch.cpp index 1ffbfe3657c6cd..950be48ae5802a 100644 --- a/src/messaging/ExchangeMessageDispatch.cpp +++ b/src/messaging/ExchangeMessageDispatch.cpp @@ -51,45 +51,61 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SessionManager * sessionManager, PayloadHeader payloadHeader; payloadHeader.SetExchangeID(exchangeId).SetMessageType(protocol, type).SetInitiator(isInitiator); - // If there is a pending acknowledgment piggyback it on this message. - if (reliableMessageContext->HasPiggybackAckPending()) + if (session->AllowsMRP()) { - payloadHeader.SetAckMessageCounter(reliableMessageContext->TakePendingPeerAckMessageCounter()); - } - - if (IsReliableTransmissionAllowed() && reliableMessageContext->AutoRequestAck() && - reliableMessageContext->GetReliableMessageMgr() != nullptr && isReliableTransmission) - { - auto * reliableMessageMgr = reliableMessageContext->GetReliableMessageMgr(); - - payloadHeader.SetNeedsAck(true); - - ReliableMessageMgr::RetransTableEntry * entry = nullptr; - - // Add to Table for subsequent sending - ReturnErrorOnFailure(reliableMessageMgr->AddToRetransTable(reliableMessageContext, &entry)); - auto deleter = [reliableMessageMgr](ReliableMessageMgr::RetransTableEntry * e) { - reliableMessageMgr->ClearRetransTable(*e); - }; - std::unique_ptr entryOwner(entry, deleter); - - ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf)); - CHIP_ERROR err = sessionManager->SendPreparedMessage(session, entryOwner->retainedBuf); - err = ReliableMessageMgr::MapSendError(err, exchangeId, isInitiator); - ReturnErrorOnFailure(err); - reliableMessageMgr->StartRetransmision(entryOwner.release()); + // If there is a pending acknowledgment piggyback it on this message. + if (reliableMessageContext->HasPiggybackAckPending()) + { + payloadHeader.SetAckMessageCounter(reliableMessageContext->TakePendingPeerAckMessageCounter()); + } + + if (IsReliableTransmissionAllowed() && reliableMessageContext->AutoRequestAck() && + reliableMessageContext->GetReliableMessageMgr() != nullptr && isReliableTransmission) + { + auto * reliableMessageMgr = reliableMessageContext->GetReliableMessageMgr(); + + payloadHeader.SetNeedsAck(true); + + ReliableMessageMgr::RetransTableEntry * entry = nullptr; + + // Add to Table for subsequent sending + ReturnErrorOnFailure(reliableMessageMgr->AddToRetransTable(reliableMessageContext, &entry)); + auto deleter = [reliableMessageMgr](ReliableMessageMgr::RetransTableEntry * e) { + reliableMessageMgr->ClearRetransTable(*e); + }; + std::unique_ptr entryOwner(entry, deleter); + + ReturnErrorOnFailure( + sessionManager->PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf)); + CHIP_ERROR err = sessionManager->SendPreparedMessage(session, entryOwner->retainedBuf); + err = ReliableMessageMgr::MapSendError(err, exchangeId, isInitiator); + ReturnErrorOnFailure(err); + reliableMessageMgr->StartRetransmision(entryOwner.release()); + } + else + { + ReturnErrorOnFailure(PrepareAndSendNonMRPMessage(sessionManager, session, payloadHeader, std::move(message))); + } } else { - // If the channel itself is providing reliability, let's not request MRP acks - payloadHeader.SetNeedsAck(false); - EncryptedPacketBufferHandle preparedMessage; - ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage)); - ReturnErrorOnFailure(sessionManager->SendPreparedMessage(session, preparedMessage)); + ReturnErrorOnFailure(PrepareAndSendNonMRPMessage(sessionManager, session, payloadHeader, std::move(message))); } return CHIP_NO_ERROR; } +CHIP_ERROR ExchangeMessageDispatch::PrepareAndSendNonMRPMessage(SessionManager * sessionManager, const SessionHandle & session, + PayloadHeader & payloadHeader, + System::PacketBufferHandle && message) +{ + payloadHeader.SetNeedsAck(false); + EncryptedPacketBufferHandle preparedMessage; + ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage)); + ReturnErrorOnFailure(sessionManager->SendPreparedMessage(session, preparedMessage)); + + return CHIP_NO_ERROR; +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeMessageDispatch.h b/src/messaging/ExchangeMessageDispatch.h index 35f12e06e80d36..8585ebd422fc5f 100644 --- a/src/messaging/ExchangeMessageDispatch.h +++ b/src/messaging/ExchangeMessageDispatch.h @@ -48,6 +48,10 @@ class ExchangeMessageDispatch // TODO: remove IsReliableTransmissionAllowed, this function should be provided over session. virtual bool IsReliableTransmissionAllowed() const { return true; } + +private: + CHIP_ERROR PrepareAndSendNonMRPMessage(SessionManager * sessionManager, const SessionHandle & session, + PayloadHeader & payloadHeader, System::PacketBufferHandle && message); }; } // namespace Messaging diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index dddc9c6fd4ce8f..2ca22031094bb7 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -372,8 +372,10 @@ void ExchangeManager::SendStandaloneAckIfNeeded(const PacketHeader & packetHeade const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf) { - // If we need to send a StandaloneAck, create a EphemeralExchange for the purpose to send the StandaloneAck - if (!payloadHeader.NeedsAck()) + + // If using the MRP protocol and we need to send a StandaloneAck, create an EphemeralExchange to send + // the StandaloneAck. + if (!session->AllowsMRP() || !payloadHeader.NeedsAck()) return; // If rcvd msg is from initiator then this exchange is created as not Initiator. diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h index 608bc78891b0e0..f20ff069ab029f 100644 --- a/src/transport/GroupSession.h +++ b/src/transport/GroupSession.h @@ -57,7 +57,8 @@ class IncomingGroupSession : public Session, public ReferenceCounted