Skip to content

Commit

Permalink
Fix subscription liveness timeout computation to include MRP backoff. (
Browse files Browse the repository at this point in the history
…#25593)

Right now the amount of time we allow for receiving a subscription report, after
the max-interval has elapsed, is computed by just multiplying our active
liveness timeout by the number of MRP retries.  For a typical always-active
client (hence 300ms active interval) that means 1500ms.

But an actual sender would do backoff in between the MRP messages, so would not
actually give up within 1500ms.  We should be taking that backoff into account
when computing the time we allow to receive the message.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Aug 3, 2023
1 parent 5525d6e commit 6222756
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 9 deletions.
9 changes: 8 additions & 1 deletion src/app/ReadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <lib/core/TLVTypes.h>
#include <lib/support/FibonacciUtils.h>
#include <messaging/ReliableMessageMgr.h>
#include <messaging/ReliableMessageProtocolConfig.h>

namespace chip {
namespace app {
Expand Down Expand Up @@ -808,11 +809,17 @@ CHIP_ERROR ReadClient::RefreshLivenessCheckTimer()
// 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.
//
// Make sure to use the retransmission computation that includes backoff. For purposes of that computation, treat us as
// active now (since we are right now sending/receiving messages), and use the default "how long are we guaranteed to stay
// active" threshold for now.
//
// 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.
//
const auto & ourMrpConfig = GetDefaultMRPConfig();
auto publisherTransmissionTimeout =
GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig()).mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
System::SystemClock().GetMonotonicTimestamp(), Transport::kMinActiveTime);
timeout = System::Clock::Seconds16(mMaxInterval) + publisherTransmissionTimeout;
}

Expand Down
34 changes: 26 additions & 8 deletions src/controller/tests/data_model/TestRead.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,10 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback
// of reads in parallel and wait for them all to succeed.
static void SubscribeThenReadHelper(nlTestSuite * apSuite, TestContext & aCtx, size_t aSubscribeCount, size_t aReadCount);

// Compute the amount of time it would take a subscription with a given
// max-interval to time out.
static System::Clock::Timeout ComputeSubscriptionTimeout(System::Clock::Seconds16 aMaxInterval);

bool mEmitSubscriptionError = false;
int32_t mNumActiveSubscriptions = 0;
bool mAlterSubscriptionIntervals = false;
Expand Down Expand Up @@ -1684,7 +1688,9 @@ void TestReadInteraction::TestResubscribeAttributeTimeout(nlTestSuite * apSuite,
attributePathParams[0].mClusterId = app::Clusters::UnitTesting::Id;
attributePathParams[0].mAttributeId = app::Clusters::UnitTesting::Attributes::Boolean::Id;

readPrepareParams.mMaxIntervalCeilingSeconds = 1;
constexpr uint16_t maxIntervalCeilingSeconds = 1;

readPrepareParams.mMaxIntervalCeilingSeconds = maxIntervalCeilingSeconds;

auto err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -1701,10 +1707,9 @@ void TestReadInteraction::TestResubscribeAttributeTimeout(nlTestSuite * apSuite,
//
// Disable packet transmission, and drive IO till we have reported a re-subscription attempt.
//
// 1.5s should cover the liveness timeout in the client of 1s max interval + 50ms ACK timeout.
//
ctx.GetLoopback().mNumMessagesToDrop = chip::Test::LoopbackTransport::kUnlimitedMessageCount;
ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1500),
ctx.GetIOContext().DriveIOUntil(ComputeSubscriptionTimeout(System::Clock::Seconds16(maxIntervalCeilingSeconds)),
[&]() { return callback.mOnResubscriptionsAttempted > 0; });

NL_TEST_ASSERT(apSuite, callback.mOnResubscriptionsAttempted == 1);
Expand Down Expand Up @@ -1763,7 +1768,8 @@ void TestReadInteraction::TestSubscribeAttributeTimeout(nlTestSuite * apSuite, v
//
// Request a max interval that's very small to reduce time to discovering a liveness failure.
//
readPrepareParams.mMaxIntervalCeilingSeconds = 1;
constexpr uint16_t maxIntervalCeilingSeconds = 1;
readPrepareParams.mMaxIntervalCeilingSeconds = maxIntervalCeilingSeconds;

auto err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -1782,11 +1788,11 @@ void TestReadInteraction::TestSubscribeAttributeTimeout(nlTestSuite * apSuite, v

//
// Drive IO until we get an error on the subscription, which should be caused
// by the liveness timer firing within ~1s of the establishment of the subscription.
// by the liveness timer firing once we hit our max-interval plus
// retransmit timeouts.
//
// 1.5s should cover the liveness timeout in the client of 1s max interval + 50ms ACK timeout.
//
ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1500), [&]() { return callback.mOnError >= 1; });
ctx.GetIOContext().DriveIOUntil(ComputeSubscriptionTimeout(System::Clock::Seconds16(maxIntervalCeilingSeconds)),
[&]() { return callback.mOnError >= 1; });

NL_TEST_ASSERT(apSuite, callback.mOnError == 1);
NL_TEST_ASSERT(apSuite, callback.mLastError == CHIP_ERROR_TIMEOUT);
Expand Down Expand Up @@ -4617,6 +4623,18 @@ void TestReadInteraction::TestReadHandler_KeepSubscriptionTest(nlTestSuite * apS
ctx.DrainAndServiceIO();
}

System::Clock::Timeout TestReadInteraction::ComputeSubscriptionTimeout(System::Clock::Seconds16 aMaxInterval)
{
// Add 50ms of slack to our max interval to make sure we hit the
// subscription liveness timer.
const auto & ourMrpConfig = GetDefaultMRPConfig();
auto publisherTransmissionTimeout =
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
System::SystemClock().GetMonotonicTimestamp(), Transport::kMinActiveTime);

return publisherTransmissionTimeout + aMaxInterval + System::Clock::Milliseconds32(50);
}

// clang-format off
const nlTest sTests[] =
{
Expand Down

0 comments on commit 6222756

Please sign in to comment.