From 62227561c24621efc4c319dee1dc9fdd6b827799 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 9 Mar 2023 13:14:12 -0500 Subject: [PATCH] Fix subscription liveness timeout computation to include MRP backoff. (#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. --- src/app/ReadClient.cpp | 9 +++++- src/controller/tests/data_model/TestRead.cpp | 34 +++++++++++++++----- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index cbd131f288c3b0..939c599704724d 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -31,6 +31,7 @@ #include #include #include +#include namespace chip { namespace app { @@ -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; } diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 1bfbac35ef93b4..c9fb756200e273 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -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; @@ -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); @@ -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); @@ -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); @@ -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); @@ -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[] = {