Skip to content

Commit 5fd234d

Browse files
yunhanw-googlerestyled-commitsbzbarsky-apple
authored
Fix MRP baseTimeout calculation for LIT (#37696) (#38062)
* Fix MRP baseTimeout calculation when it is possible the other end fall asleep * use min(SAI, SII) for mrp timeout when clock runs into idle * Restyled by clang-format * update mrp calculation -- Use 15 seconds to determine whether device is in LIT mode, then if yes, set the timeout base as SAI if GetRemoteMRPConfig().mIdleRetransTimeout.count() > sitIcdSlowPollMaximum.count() * Address comments * address comments * Restyled by whitespace * address comments * Restyled by whitespace * Restyled by clang-format * Update src/transport/UnauthenticatedSessionTable.h * Update src/transport/Session.h * Update src/protocols/secure_channel/CASESession.cpp * Update src/transport/SecureSession.h * Update src/protocols/secure_channel/CASESession.cpp * Update src/app/ReadClient.cpp * Update src/messaging/ReliableMessageProtocolConfig.cpp * Update src/messaging/ReliableMessageProtocolConfig.h * address comments * address comments * update true isFirstMessageOnExchange for all ComputeRoundTripTimeout caller * Restyled by whitespace * Restyled by clang-format * fix compilation error * address comments * cleanup * add parameter isFirstMessageOnExchange in GetMessageReceiptTimeout * Restyled by clang-format --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 91eab26 commit 5fd234d

17 files changed

+83
-51
lines changed

src/app/CommandSender.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ CHIP_ERROR CommandSender::SendCommandRequestInternal(const SessionHandle & sessi
125125
mExchangeCtx.Grab(exchange);
126126
VerifyOrReturnError(!mExchangeCtx->IsGroupExchangeContext(), CHIP_ERROR_INVALID_MESSAGE_TYPE);
127127

128-
mExchangeCtx->SetResponseTimeout(timeout.ValueOr(session->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime)));
128+
mExchangeCtx->SetResponseTimeout(
129+
timeout.ValueOr(session->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime, true /*isFirstMessageOnExchange*/)));
129130

130131
if (mTimedInvokeTimeoutMs.HasValue())
131132
{

src/app/ReadClient.cpp

+7-4
Original file line numberDiff line numberDiff line change
@@ -987,12 +987,15 @@ CHIP_ERROR ReadClient::ComputeLivenessCheckTimerTimeout(System::Clock::Timeout *
987987
// So recompute the round-trip timeout directly. Assume MRP, since in practice that is likely what is happening.
988988
auto & peerMRPConfig = mReadPrepareParams.mSessionHolder->GetRemoteMRPConfig();
989989
// Peer will assume we are idle (hence we pass kZero to GetMessageReceiptTimeout()), but will assume we treat it as active
990-
// for the response, so to match the retransmission timeout computation for the message back to the peeer, we should treat
991-
// it as active.
992-
auto roundTripTimeout = mReadPrepareParams.mSessionHolder->GetMessageReceiptTimeout(System::Clock::kZero) +
990+
// for the response, so to match the retransmission timeout computation for the message back to the peer, we should treat
991+
// it as active and handling non-initial message, isFirstMessageOnExchange needs to be set as false for
992+
// GetRetransmissionTimeout.
993+
auto roundTripTimeout =
994+
mReadPrepareParams.mSessionHolder->GetMessageReceiptTimeout(System::Clock::kZero, true /*isFirstMessageOnExchange*/) +
993995
kExpectedIMProcessingTime +
994996
GetRetransmissionTimeout(peerMRPConfig.mActiveRetransTimeout, peerMRPConfig.mIdleRetransTimeout,
995-
System::SystemClock().GetMonotonicTimestamp(), peerMRPConfig.mActiveThresholdTime);
997+
System::SystemClock().GetMonotonicTimestamp(), peerMRPConfig.mActiveThresholdTime,
998+
false /*isFirstMessageOnExchange*/);
996999
*aTimeout = System::Clock::Seconds16(mMaxInterval) + roundTripTimeout;
9971000
return CHIP_NO_ERROR;
9981001
}

src/app/TimedHandler.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,9 @@ CHIP_ERROR TimedHandler::HandleTimedRequestAction(Messaging::ExchangeContext * a
127127
// will send nothing and the other side will have to time out to realize
128128
// it's missed its window).
129129
auto delay = System::Clock::Milliseconds32(timeoutMs);
130-
aExchangeContext->SetResponseTimeout(
131-
std::max(delay, aExchangeContext->GetSessionHandle()->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime)));
130+
aExchangeContext->SetResponseTimeout(std::max(delay,
131+
aExchangeContext->GetSessionHandle()->ComputeRoundTripTimeout(
132+
app::kExpectedIMProcessingTime, false /*isFirstMessageOnExchange*/)));
132133
ReturnErrorOnFailure(StatusResponse::Send(Status::Success, aExchangeContext, /* aExpectResponse = */ true));
133134

134135
// Now just wait for the client.

src/controller/AutoCommissioner.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -651,7 +651,7 @@ Optional<System::Clock::Timeout> AutoCommissioner::GetCommandTimeout(DeviceProxy
651651
auto sessionHandle = device->GetSecureSession();
652652
if (sessionHandle.HasValue())
653653
{
654-
timeout = sessionHandle.Value()->ComputeRoundTripTimeout(timeout);
654+
timeout = sessionHandle.Value()->ComputeRoundTripTimeout(timeout, true /*isFirstMessageOnExchange*/);
655655
}
656656

657657
// Enforce the spec minimal timeout. Maybe this enforcement should live in

src/controller/python/chip/utils/DeviceProxyUtils.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ uint32_t pychip_DeviceProxy_ComputeRoundTripTimeout(DeviceProxy * device, uint32
7171

7272
return deviceProxy->GetSecureSession()
7373
.Value()
74-
->ComputeRoundTripTimeout(System::Clock::Milliseconds32(upperLayerProcessingTimeoutMs))
74+
->ComputeRoundTripTimeout(System::Clock::Milliseconds32(upperLayerProcessingTimeoutMs), true /*isFirstMessageOnExchange*/)
7575
.count();
7676
}
7777

src/controller/tests/TestWriteChunking.cpp

+4-3
Original file line numberDiff line numberDiff line change
@@ -566,9 +566,10 @@ void TestWriteChunking::RunTest(Instructions instructions)
566566
err = writeClient->SendWriteRequest(sessionHandle);
567567
EXPECT_EQ(err, CHIP_NO_ERROR);
568568

569-
GetIOContext().DriveIOUntil(sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime) +
570-
System::Clock::Seconds16(1),
571-
[&]() { return GetExchangeManager().GetNumActiveExchanges() == 0; });
569+
GetIOContext().DriveIOUntil(
570+
sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime, true /*isFirstMessageOnExchange*/) +
571+
System::Clock::Seconds16(1),
572+
[&]() { return GetExchangeManager().GetNumActiveExchanges() == 0; });
572573

573574
EXPECT_EQ(onGoingPath, app::ConcreteAttributePath());
574575
EXPECT_EQ(status.size(), instructions.expectedStatus.size());

src/controller/tests/data_model/TestRead.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -4783,10 +4783,10 @@ System::Clock::Timeout TestRead::ComputeSubscriptionTimeout(System::Clock::Secon
47834783
// Add 1000ms of slack to our max interval to make sure we hit the
47844784
// subscription liveness timer. 100ms was tried in the past and is not
47854785
// sufficient: our process can easily lose the timeslice for 100ms.
4786-
const auto & ourMrpConfig = GetDefaultMRPConfig();
4787-
auto publisherTransmissionTimeout =
4788-
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
4789-
System::SystemClock().GetMonotonicTimestamp(), ourMrpConfig.mActiveThresholdTime);
4786+
const auto & ourMrpConfig = GetDefaultMRPConfig();
4787+
auto publisherTransmissionTimeout = GetRetransmissionTimeout(
4788+
ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout, System::SystemClock().GetMonotonicTimestamp(),
4789+
ourMrpConfig.mActiveThresholdTime, true /* isFirstMessageOnExchange */);
47904790

47914791
return publisherTransmissionTimeout + aMaxInterval + System::Clock::Milliseconds32(1000);
47924792
}

src/darwin/Framework/CHIP/MTRBaseDevice.mm

+1-1
Original file line numberDiff line numberDiff line change
@@ -1475,7 +1475,7 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
14751475
// Clamp to a number of seconds that will not overflow 32-bit
14761476
// int when converted to ms.
14771477
auto serverTimeoutInSeconds = System::Clock::Seconds16(serverSideProcessingTimeout.unsignedShortValue);
1478-
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(serverTimeoutInSeconds));
1478+
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(serverTimeoutInSeconds, true /*isFirstMessageOnExchange*/));
14791479
}
14801480
ReturnErrorOnFailure(commandSender->SendCommandRequest(session, invokeTimeout));
14811481

src/messaging/ExchangeContext.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ void ExchangeContext::SetResponseExpected(bool inResponseExpected)
7777

7878
void ExchangeContext::UseSuggestedResponseTimeout(Timeout applicationProcessingTimeout)
7979
{
80-
SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout));
80+
SetResponseTimeout(mSession->ComputeRoundTripTimeout(applicationProcessingTimeout, !HasReceivedAtLeastOneMessage()));
8181
}
8282

8383
void ExchangeContext::SetResponseTimeout(Timeout timeout)

src/messaging/ReliableMessageProtocolConfig.cpp

+10-2
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,8 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
126126
}
127127

128128
System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
129-
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold)
129+
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold,
130+
bool isFirstMessageOnExchange)
130131
{
131132
auto timeSinceLastActivity = (System::SystemClock().GetMonotonicTimestamp() - lastActivityTime);
132133

@@ -135,7 +136,14 @@ System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInt
135136
System::Clock::Timestamp timeout(0);
136137
for (uint8_t i = 0; i < CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1; i++)
137138
{
138-
auto baseInterval = ((timeSinceLastActivity + timeout) < activityThreshold) ? activeInterval : idleInterval;
139+
auto baseInterval = activeInterval;
140+
// If we are calculating the timeout for the initial message, we never know whether the peer is active or not, choose
141+
// active/idle interval from PeerActiveMode of session per 4.11.2.1. Retransmissions.
142+
// If we are calculating the timeout for response message, we know the peer is active, always choose active interval.
143+
if (isFirstMessageOnExchange)
144+
{
145+
baseInterval = ((timeSinceLastActivity + timeout) < activityThreshold) ? activeInterval : idleInterval;
146+
}
139147
timeout += Messaging::ReliableMessageMgr::GetBackoff(baseInterval, i, /* computeMaxPossible */ true);
140148
}
141149

src/messaging/ReliableMessageProtocolConfig.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,12 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
257257
* @param[in] idleInterval The idle interval to use for the backoff calculation.
258258
* @param[in] lastActivityTime The last time some activity has been recorded.
259259
* @param[in] activityThreshold The activity threshold for a node to be considered active.
260-
*
260+
* @param[in] isFirstMessageOnExchange Indicates whether this is for the initial message on an exchange.
261261
* @return The maximum transmission time
262262
*/
263263
System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
264-
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold);
264+
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold,
265+
bool isFirstMessageOnExchange);
265266

266267
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
267268

src/protocols/secure_channel/CASESession.cpp

+13-10
Original file line numberDiff line numberDiff line change
@@ -2426,34 +2426,37 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
24262426

24272427
namespace {
24282428
System::Clock::Timeout ComputeRoundTripTimeout(ExchangeContext::Timeout serverProcessingTime,
2429-
const ReliableMessageProtocolConfig & remoteMrpConfig)
2429+
const ReliableMessageProtocolConfig & remoteMrpConfig, bool isFirstMessageOnExchange)
24302430
{
24312431
// TODO: This is duplicating logic from Session::ComputeRoundTripTimeout. Unfortunately, it's called by
24322432
// consumers who do not have a session.
24332433
const auto & maybeLocalMRPConfig = GetLocalMRPConfig();
24342434
const auto & defaultMRRPConfig = GetDefaultMRPConfig();
24352435
const auto & localMRPConfig = maybeLocalMRPConfig.ValueOr(defaultMRRPConfig);
24362436
return GetRetransmissionTimeout(remoteMrpConfig.mActiveRetransTimeout, remoteMrpConfig.mIdleRetransTimeout,
2437-
// Assume peer is idle, as a worst-case assumption (probably true for
2438-
// Sigma1, since that will be our initial message on the session, but less
2439-
// so for Sigma2).
2440-
System::Clock::kZero, remoteMrpConfig.mActiveThresholdTime) +
2437+
// The activity time only matters when isFirstMessageOnExchange is false,
2438+
// which only happens for Sigma1. In that case, assume peer is idle,
2439+
// as a worst-case assumption, and pass System::Clock::kZero.
2440+
System::Clock::kZero, remoteMrpConfig.mActiveThresholdTime, isFirstMessageOnExchange) +
24412441
serverProcessingTime +
24422442
GetRetransmissionTimeout(localMRPConfig.mActiveRetransTimeout, localMRPConfig.mIdleRetransTimeout,
2443-
// Peer will assume we are active, since it's
2444-
// responding to our message.
2445-
System::SystemClock().GetMonotonicTimestamp(), localMRPConfig.mActiveThresholdTime);
2443+
// Peer will be responding to our message, so isFirstMessageOnExchange should be false
2444+
// and the timestamp does not matter.
2445+
System::SystemClock().GetMonotonicTimestamp(), localMRPConfig.mActiveThresholdTime,
2446+
false /*isFirstMessageOnExchange*/);
24462447
}
24472448
} // anonymous namespace
24482449

24492450
System::Clock::Timeout CASESession::ComputeSigma1ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig)
24502451
{
2451-
return ComputeRoundTripTimeout(kExpectedSigma1ProcessingTime, remoteMrpConfig);
2452+
// Sigma1 is the first message on the exchange.
2453+
return ComputeRoundTripTimeout(kExpectedSigma1ProcessingTime, remoteMrpConfig, true /*isFirstMessageOnExchange*/);
24522454
}
24532455

24542456
System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig)
24552457
{
2456-
return ComputeRoundTripTimeout(kExpectedHighProcessingTime, remoteMrpConfig);
2458+
// Sigma2 is never the first message on the exchange.
2459+
return ComputeRoundTripTimeout(kExpectedHighProcessingTime, remoteMrpConfig, false /*isFirstMessageOnExchange*/);
24572460
}
24582461

24592462
bool CASESession::InvokeBackgroundWorkWatchdog()

src/transport/GroupSession.h

+6-4
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,14 @@ class IncomingGroupSession : public Session, public ReferenceCounted<IncomingGro
6969

7070
System::Clock::Timestamp GetMRPBaseTimeout() const override { return System::Clock::kZero; }
7171

72-
System::Clock::Milliseconds32 GetAckTimeout() const override
72+
System::Clock::Milliseconds32 GetAckTimeout(bool isFirstMessageOnExchange) const override
7373
{
7474
VerifyOrDie(false);
7575
return System::Clock::Timeout();
7676
}
7777

78-
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity) const override
78+
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity,
79+
bool isFirstMessageOnExchange) const override
7980
{
8081
// There are no timeouts for group sessions.
8182
VerifyOrDie(false);
@@ -128,13 +129,14 @@ class OutgoingGroupSession : public Session, public ReferenceCounted<OutgoingGro
128129

129130
System::Clock::Timestamp GetMRPBaseTimeout() const override { return System::Clock::kZero; }
130131

131-
System::Clock::Milliseconds32 GetAckTimeout() const override
132+
System::Clock::Milliseconds32 GetAckTimeout(bool isFirstMessageOnExchange) const override
132133
{
133134
VerifyOrDie(false);
134135
return System::Clock::Timeout();
135136
}
136137

137-
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity) const override
138+
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity,
139+
bool isFirstMessageOnExchange) const override
138140
{
139141
// There are no timeouts for group sessions.
140142
VerifyOrDie(false);

src/transport/SecureSession.h

+6-4
Original file line numberDiff line numberDiff line change
@@ -162,14 +162,15 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
162162

163163
bool AllowsLargePayload() const override { return GetPeerAddress().GetTransportType() == Transport::Type::kTcp; }
164164

165-
System::Clock::Milliseconds32 GetAckTimeout() const override
165+
System::Clock::Milliseconds32 GetAckTimeout(bool isFirstMessageOnExchange) const override
166166
{
167167
switch (mPeerAddress.GetTransportType())
168168
{
169169
case Transport::Type::kUdp: {
170170
const ReliableMessageProtocolConfig & remoteMRPConfig = mRemoteSessionParams.GetMRPConfig();
171171
return GetRetransmissionTimeout(remoteMRPConfig.mActiveRetransTimeout, remoteMRPConfig.mIdleRetransTimeout,
172-
GetLastPeerActivityTime(), remoteMRPConfig.mActiveThresholdTime);
172+
GetLastPeerActivityTime(), remoteMRPConfig.mActiveThresholdTime,
173+
isFirstMessageOnExchange);
173174
}
174175
case Transport::Type::kTcp:
175176
return System::Clock::Seconds16(30);
@@ -181,7 +182,8 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
181182
return System::Clock::Timeout();
182183
}
183184

184-
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity) const override
185+
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity,
186+
bool isFirstMessageOnExchange) const override
185187
{
186188
switch (mPeerAddress.GetTransportType())
187189
{
@@ -190,7 +192,7 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
190192
const auto & defaultMRRPConfig = GetDefaultMRPConfig();
191193
const auto & localMRPConfig = maybeLocalMRPConfig.ValueOr(defaultMRRPConfig);
192194
return GetRetransmissionTimeout(localMRPConfig.mActiveRetransTimeout, localMRPConfig.mIdleRetransTimeout,
193-
ourLastActivity, localMRPConfig.mActiveThresholdTime);
195+
ourLastActivity, localMRPConfig.mActiveThresholdTime, isFirstMessageOnExchange);
194196
}
195197
case Transport::Type::kTcp:
196198
return System::Clock::Seconds16(30);

src/transport/Session.cpp

+6-4
Original file line numberDiff line numberDiff line change
@@ -64,16 +64,18 @@ const OutgoingGroupSession * Session::AsConstOutgoingGroupSession() const
6464
return static_cast<const OutgoingGroupSession *>(this);
6565
}
6666

67-
System::Clock::Timeout Session::ComputeRoundTripTimeout(System::Clock::Timeout upperlayerProcessingTimeout)
67+
System::Clock::Timeout Session::ComputeRoundTripTimeout(System::Clock::Timeout upperlayerProcessingTimeout,
68+
bool isFirstMessageOnExchange)
6869
{
6970
if (IsGroupSession())
7071
{
7172
return System::Clock::kZero;
7273
}
7374

74-
// Treat us as active for purposes of GetMessageReceiptTimeout(), since the
75-
// other side would be responding to our message.
76-
return GetAckTimeout() + upperlayerProcessingTimeout + GetMessageReceiptTimeout(System::SystemClock().GetMonotonicTimestamp());
75+
// Treat us as active for purposes of GetMessageReceiptTimeout(), pass false into GetMessageReceiptTimeout to
76+
// indicate we are processing non-initial message since the other side would be responding to our message.
77+
return GetAckTimeout(isFirstMessageOnExchange) + upperlayerProcessingTimeout +
78+
GetMessageReceiptTimeout(System::SystemClock().GetMonotonicTimestamp(), false /*isFirstMessageOnExchange*/);
7779
}
7880

7981
uint16_t Session::SessionIdForLogging() const

0 commit comments

Comments
 (0)