From 1ca09a484b0dc92719b51cc2d5baba6d4275744c Mon Sep 17 00:00:00 2001
From: Jerry Johns <johnsj@google.com>
Date: Tue, 20 Sep 2022 10:23:03 -0400
Subject: [PATCH] [BUG] Fix ReadClient to use the right parameters when
computing the liveness timeout (#22699)
* Fix ReadClient to use the right parameters when computing the liveness
timeout
This fixes the logic in ReadClient to use the local IDLE interval when
computing the liveness timeout instead of the IDLE interval of our peer.
Testing:
Using the REPL and the liveness timer print, confirmed the fixes are
indeed applied.
Restyled by whitespace
Restyled by clang-format
Build fixes
* Review feedback
---
src/app/ReadClient.cpp | 15 ++++++-
.../ReliableMessageProtocolConfig.cpp | 29 +++++++++++++
src/messaging/ReliableMessageProtocolConfig.h | 20 +++++++++
src/messaging/tests/MessagingContext.cpp | 41 +++++++++++++++----
src/messaging/tests/MessagingContext.h | 6 +++
5 files changed, 102 insertions(+), 9 deletions(-)
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
index 61a733af974d6c..f981cb18217c6a 100644
--- a/src/app/ReadClient.cpp
+++ b/src/app/ReadClient.cpp
@@ -30,6 +30,7 @@
#include <assert.h>
#include <lib/core/CHIPTLVTypes.h>
#include <lib/support/FibonacciUtils.h>
+#include <messaging/ReliableMessageMgr.h>
namespace chip {
namespace app {
@@ -798,7 +799,19 @@ CHIP_ERROR ReadClient::RefreshLivenessCheckTimer()
else
{
VerifyOrReturnError(mReadPrepareParams.mSessionHolder, CHIP_ERROR_INCORRECT_STATE);
- timeout = System::Clock::Seconds16(mMaxInterval) + mReadPrepareParams.mSessionHolder->GetAckTimeout();
+
+ //
+ // To calculate the duration we're willing to wait for a report to come to us, we take into account the maximum interval of
+ // the subscription AND the time it takes for the report to make it to us in the worst case. This latter bit involves
+ // computing the Ack timeout from the publisher for the ReportData message being sent to us using our IDLE interval as the
+ // basis for that computation.
+ //
+ // TODO: We need to find a good home for this logic that will correctly compute this based on transport. For now, this will
+ // suffice since we don't use TCP as a transport currently and subscriptions over BLE aren't really a thing.
+ //
+ auto publisherTransmissionTimeout =
+ GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()).mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
+ timeout = System::Clock::Seconds16(mMaxInterval) + publisherTransmissionTimeout;
}
// EFR32/MBED/INFINION/K32W's chrono count return long unsinged, but other platform returns unsigned
diff --git a/src/messaging/ReliableMessageProtocolConfig.cpp b/src/messaging/ReliableMessageProtocolConfig.cpp
index ef9110964adc10..f9358e50227f94 100644
--- a/src/messaging/ReliableMessageProtocolConfig.cpp
+++ b/src/messaging/ReliableMessageProtocolConfig.cpp
@@ -31,6 +31,23 @@ namespace chip {
using namespace System::Clock::Literals;
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+static Optional<System::Clock::Timeout> idleRetransTimeoutOverride = NullOptional;
+static Optional<System::Clock::Timeout> activeRetransTimeoutOverride = NullOptional;
+
+void OverrideLocalMRPConfig(System::Clock::Timeout idleRetransTimeout, System::Clock::Timeout activeRetransTimeout)
+{
+ idleRetransTimeoutOverride.SetValue(idleRetransTimeout);
+ activeRetransTimeoutOverride.SetValue(activeRetransTimeout);
+}
+
+void ClearLocalMRPConfigOverride()
+{
+ activeRetransTimeoutOverride.ClearValue();
+ idleRetransTimeoutOverride.ClearValue();
+}
+#endif
+
ReliableMessageProtocolConfig GetDefaultMRPConfig()
{
// Default MRP intervals are defined in spec <2.11.3. Parameters and Constants>
@@ -55,6 +72,18 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
}
#endif
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+ if (idleRetransTimeoutOverride.HasValue())
+ {
+ config.mIdleRetransTimeout = idleRetransTimeoutOverride.Value();
+ }
+
+ if (activeRetransTimeoutOverride.HasValue())
+ {
+ config.mActiveRetransTimeout = activeRetransTimeoutOverride.Value();
+ }
+#endif
+
return (config == GetDefaultMRPConfig()) ? Optional<ReliableMessageProtocolConfig>::Missing()
: Optional<ReliableMessageProtocolConfig>::Value(config);
}
diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h
index 2e253c10bdc3ac..ad84a19fa62fdb 100644
--- a/src/messaging/ReliableMessageProtocolConfig.h
+++ b/src/messaging/ReliableMessageProtocolConfig.h
@@ -156,4 +156,24 @@ ReliableMessageProtocolConfig GetDefaultMRPConfig();
*/
Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+
+/**
+ * @brief
+ *
+ * Overrides the local idle and active retransmission timeout parameters (which are usually set through compile
+ * time defines). This is reserved for tests that need the ability to set these at runtime to make certain test scenarios possible.
+ *
+ */
+void OverrideLocalMRPConfig(System::Clock::Timeout idleRetransTimeout, System::Clock::Timeout activeRetransTimeout);
+
+/**
+ * @brief
+ *
+ * Disables the overrides set previously in OverrideLocalMRPConfig().
+ *
+ */
+void ClearLocalMRPConfigOverride();
+#endif
+
} // namespace chip
diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp
index 6650b6c42f42a5..a6955aaf88acaa 100644
--- a/src/messaging/tests/MessagingContext.cpp
+++ b/src/messaging/tests/MessagingContext.cpp
@@ -95,6 +95,11 @@ void MessagingContext::ShutdownAndRestoreExisting(MessagingContext & existing)
existing.mTransport->SetSessionManager(&existing.GetSecureSessionManager());
}
+using namespace System::Clock::Literals;
+
+constexpr chip::System::Clock::Timeout MessagingContext::kResponsiveIdleRetransTimeout;
+constexpr chip::System::Clock::Timeout MessagingContext::kResponsiveActiveRetransTimeout;
+
void MessagingContext::SetMRPMode(MRPMode mode)
{
if (mode == MRPMode::kDefault)
@@ -103,17 +108,37 @@ void MessagingContext::SetMRPMode(MRPMode mode)
mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(GetDefaultMRPConfig());
+
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+ ClearLocalMRPConfigOverride();
+#else
+ //
+ // A test is calling this function assuming the overrides above are going to work
+ // when in fact, they won't because the compile flag is not set correctly.
+ //
+ VerifyOrDie(false);
+#endif
}
else
{
- mSessionBobToAlice->AsSecureSession()->SetRemoteMRPConfig(
- ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
- mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(
- ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
- mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(
- ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
- mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(
- ReliableMessageProtocolConfig(System::Clock::Milliseconds32(10), System::Clock::Milliseconds32(10)));
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+ OverrideLocalMRPConfig(MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout);
+#else
+ //
+ // A test is calling this function assuming the overrides above are going to work
+ // when in fact, they won't because the compile flag is not set correctly.
+ //
+ VerifyOrDie(false);
+#endif
+
+ mSessionBobToAlice->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+ MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
+ mSessionAliceToBob->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+ MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
+ mSessionCharlieToDavid->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+ MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
+ mSessionDavidToCharlie->AsSecureSession()->SetRemoteMRPConfig(ReliableMessageProtocolConfig(
+ MessagingContext::kResponsiveIdleRetransTimeout, MessagingContext::kResponsiveActiveRetransTimeout));
}
}
diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h
index 8d33c50b6176b9..fea896bdaa153a 100644
--- a/src/messaging/tests/MessagingContext.h
+++ b/src/messaging/tests/MessagingContext.h
@@ -85,6 +85,12 @@ class MessagingContext : public PlatformMemoryUser
// i.e IDLE = 10ms, ACTIVE = 10ms
};
+ //
+ // See above for a description of the values used.
+ //
+ static constexpr System::Clock::Timeout kResponsiveIdleRetransTimeout = System::Clock::Milliseconds32(10);
+ static constexpr System::Clock::Timeout kResponsiveActiveRetransTimeout = System::Clock::Milliseconds32(10);
+
MessagingContext() :
mInitialized(false), mAliceAddress(Transport::PeerAddress::UDP(GetAddress(), CHIP_PORT + 1)),
mBobAddress(Transport::PeerAddress::UDP(GetAddress(), CHIP_PORT))