Skip to content

Commit d815fde

Browse files
yunhanw-googlerestyled-commitsbzbarsky-apple
authored
Fix MRP baseTimeout calculation for LIT (#37696)
* 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 Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/Session.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/protocols/secure_channel/CASESession.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/protocols/secure_channel/CASESession.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/ReadClient.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/messaging/ReliableMessageProtocolConfig.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/messaging/ReliableMessageProtocolConfig.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * 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 648e3f6 commit d815fde

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
@@ -989,12 +989,15 @@ CHIP_ERROR ReadClient::ComputeLivenessCheckTimerTimeout(System::Clock::Timeout *
989989
// So recompute the round-trip timeout directly. Assume MRP, since in practice that is likely what is happening.
990990
auto & peerMRPConfig = mReadPrepareParams.mSessionHolder->GetRemoteMRPConfig();
991991
// Peer will assume we are idle (hence we pass kZero to GetMessageReceiptTimeout()), but will assume we treat it as active
992-
// for the response, so to match the retransmission timeout computation for the message back to the peeer, we should treat
993-
// it as active.
994-
auto roundTripTimeout = mReadPrepareParams.mSessionHolder->GetMessageReceiptTimeout(System::Clock::kZero) +
992+
// for the response, so to match the retransmission timeout computation for the message back to the peer, we should treat
993+
// it as active and handling non-initial message, isFirstMessageOnExchange needs to be set as false for
994+
// GetRetransmissionTimeout.
995+
auto roundTripTimeout =
996+
mReadPrepareParams.mSessionHolder->GetMessageReceiptTimeout(System::Clock::kZero, true /*isFirstMessageOnExchange*/) +
995997
kExpectedIMProcessingTime +
996998
GetRetransmissionTimeout(peerMRPConfig.mActiveRetransTimeout, peerMRPConfig.mIdleRetransTimeout,
997-
System::SystemClock().GetMonotonicTimestamp(), peerMRPConfig.mActiveThresholdTime);
999+
System::SystemClock().GetMonotonicTimestamp(), peerMRPConfig.mActiveThresholdTime,
1000+
false /*isFirstMessageOnExchange*/);
9981001
*aTimeout = System::Clock::Seconds16(mMaxInterval) + roundTripTimeout;
9991002
return CHIP_NO_ERROR;
10001003
}

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
@@ -623,7 +623,7 @@ Optional<System::Clock::Timeout> AutoCommissioner::GetCommandTimeout(DeviceProxy
623623
auto sessionHandle = device->GetSecureSession();
624624
if (sessionHandle.HasValue())
625625
{
626-
timeout = sessionHandle.Value()->ComputeRoundTripTimeout(timeout);
626+
timeout = sessionHandle.Value()->ComputeRoundTripTimeout(timeout, true /*isFirstMessageOnExchange*/);
627627
}
628628

629629
// 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
@@ -571,9 +571,10 @@ void TestWriteChunking::RunTest(Instructions instructions)
571571
err = writeClient->SendWriteRequest(sessionHandle);
572572
EXPECT_EQ(err, CHIP_NO_ERROR);
573573

574-
GetIOContext().DriveIOUntil(sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime) +
575-
System::Clock::Seconds16(1),
576-
[&]() { return GetExchangeManager().GetNumActiveExchanges() == 0; });
574+
GetIOContext().DriveIOUntil(
575+
sessionHandle->ComputeRoundTripTimeout(app::kExpectedIMProcessingTime, true /*isFirstMessageOnExchange*/) +
576+
System::Clock::Seconds16(1),
577+
[&]() { return GetExchangeManager().GetNumActiveExchanges() == 0; });
577578

578579
EXPECT_EQ(onGoingPath, app::ConcreteAttributePath());
579580
EXPECT_EQ(status.size(), instructions.expectedStatus.size());

src/controller/tests/data_model/TestRead.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -4681,10 +4681,10 @@ System::Clock::Timeout TestRead::ComputeSubscriptionTimeout(System::Clock::Secon
46814681
// Add 1000ms of slack to our max interval to make sure we hit the
46824682
// subscription liveness timer. 100ms was tried in the past and is not
46834683
// sufficient: our process can easily lose the timeslice for 100ms.
4684-
const auto & ourMrpConfig = GetDefaultMRPConfig();
4685-
auto publisherTransmissionTimeout =
4686-
GetRetransmissionTimeout(ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout,
4687-
System::SystemClock().GetMonotonicTimestamp(), ourMrpConfig.mActiveThresholdTime);
4684+
const auto & ourMrpConfig = GetDefaultMRPConfig();
4685+
auto publisherTransmissionTimeout = GetRetransmissionTimeout(
4686+
ourMrpConfig.mActiveRetransTimeout, ourMrpConfig.mIdleRetransTimeout, System::SystemClock().GetMonotonicTimestamp(),
4687+
ourMrpConfig.mActiveThresholdTime, true /* isFirstMessageOnExchange */);
46884688

46894689
return publisherTransmissionTimeout + aMaxInterval + System::Clock::Milliseconds32(1000);
46904690
}

src/darwin/Framework/CHIP/MTRBaseDevice.mm

+1-1
Original file line numberDiff line numberDiff line change
@@ -1526,7 +1526,7 @@ - (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
15261526
// Clamp to a number of seconds that will not overflow 32-bit
15271527
// int when converted to ms.
15281528
auto serverTimeoutInSeconds = System::Clock::Seconds16(serverSideProcessingTimeout.unsignedShortValue);
1529-
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(serverTimeoutInSeconds));
1529+
invokeTimeout.SetValue(session->ComputeRoundTripTimeout(serverTimeoutInSeconds, true /*isFirstMessageOnExchange*/));
15301530
}
15311531
ReturnErrorOnFailure(commandSender->SendCommandRequest(session, invokeTimeout));
15321532

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
@@ -2611,34 +2611,37 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
26112611

26122612
namespace {
26132613
System::Clock::Timeout ComputeRoundTripTimeout(ExchangeContext::Timeout serverProcessingTime,
2614-
const ReliableMessageProtocolConfig & remoteMrpConfig)
2614+
const ReliableMessageProtocolConfig & remoteMrpConfig, bool isFirstMessageOnExchange)
26152615
{
26162616
// TODO: This is duplicating logic from Session::ComputeRoundTripTimeout. Unfortunately, it's called by
26172617
// consumers who do not have a session.
26182618
const auto & maybeLocalMRPConfig = GetLocalMRPConfig();
26192619
const auto & defaultMRRPConfig = GetDefaultMRPConfig();
26202620
const auto & localMRPConfig = maybeLocalMRPConfig.ValueOr(defaultMRRPConfig);
26212621
return GetRetransmissionTimeout(remoteMrpConfig.mActiveRetransTimeout, remoteMrpConfig.mIdleRetransTimeout,
2622-
// Assume peer is idle, as a worst-case assumption (probably true for
2623-
// Sigma1, since that will be our initial message on the session, but less
2624-
// so for Sigma2).
2625-
System::Clock::kZero, remoteMrpConfig.mActiveThresholdTime) +
2622+
// The activity time only matters when isFirstMessageOnExchange is false,
2623+
// which only happens for Sigma1. In that case, assume peer is idle,
2624+
// as a worst-case assumption, and pass System::Clock::kZero.
2625+
System::Clock::kZero, remoteMrpConfig.mActiveThresholdTime, isFirstMessageOnExchange) +
26262626
serverProcessingTime +
26272627
GetRetransmissionTimeout(localMRPConfig.mActiveRetransTimeout, localMRPConfig.mIdleRetransTimeout,
2628-
// Peer will assume we are active, since it's
2629-
// responding to our message.
2630-
System::SystemClock().GetMonotonicTimestamp(), localMRPConfig.mActiveThresholdTime);
2628+
// Peer will be responding to our message, so isFirstMessageOnExchange should be false
2629+
// and the timestamp does not matter.
2630+
System::SystemClock().GetMonotonicTimestamp(), localMRPConfig.mActiveThresholdTime,
2631+
false /*isFirstMessageOnExchange*/);
26312632
}
26322633
} // anonymous namespace
26332634

26342635
System::Clock::Timeout CASESession::ComputeSigma1ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig)
26352636
{
2636-
return ComputeRoundTripTimeout(kExpectedSigma1ProcessingTime, remoteMrpConfig);
2637+
// Sigma1 is the first message on the exchange.
2638+
return ComputeRoundTripTimeout(kExpectedSigma1ProcessingTime, remoteMrpConfig, true /*isFirstMessageOnExchange*/);
26372639
}
26382640

26392641
System::Clock::Timeout CASESession::ComputeSigma2ResponseTimeout(const ReliableMessageProtocolConfig & remoteMrpConfig)
26402642
{
2641-
return ComputeRoundTripTimeout(kExpectedHighProcessingTime, remoteMrpConfig);
2643+
// Sigma2 is never the first message on the exchange.
2644+
return ComputeRoundTripTimeout(kExpectedHighProcessingTime, remoteMrpConfig, false /*isFirstMessageOnExchange*/);
26422645
}
26432646

26442647
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
@@ -166,14 +166,15 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
166166

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

169-
System::Clock::Milliseconds32 GetAckTimeout() const override
169+
System::Clock::Milliseconds32 GetAckTimeout(bool isFirstMessageOnExchange) const override
170170
{
171171
switch (mPeerAddress.GetTransportType())
172172
{
173173
case Transport::Type::kUdp: {
174174
const ReliableMessageProtocolConfig & remoteMRPConfig = mRemoteSessionParams.GetMRPConfig();
175175
return GetRetransmissionTimeout(remoteMRPConfig.mActiveRetransTimeout, remoteMRPConfig.mIdleRetransTimeout,
176-
GetLastPeerActivityTime(), remoteMRPConfig.mActiveThresholdTime);
176+
GetLastPeerActivityTime(), remoteMRPConfig.mActiveThresholdTime,
177+
isFirstMessageOnExchange);
177178
}
178179
case Transport::Type::kTcp:
179180
return System::Clock::Seconds16(30);
@@ -185,7 +186,8 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
185186
return System::Clock::Timeout();
186187
}
187188

188-
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity) const override
189+
System::Clock::Milliseconds32 GetMessageReceiptTimeout(System::Clock::Timestamp ourLastActivity,
190+
bool isFirstMessageOnExchange) const override
189191
{
190192
switch (mPeerAddress.GetTransportType())
191193
{
@@ -194,7 +196,7 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
194196
const auto & defaultMRRPConfig = GetDefaultMRPConfig();
195197
const auto & localMRPConfig = maybeLocalMRPConfig.ValueOr(defaultMRRPConfig);
196198
return GetRetransmissionTimeout(localMRPConfig.mActiveRetransTimeout, localMRPConfig.mIdleRetransTimeout,
197-
ourLastActivity, localMRPConfig.mActiveThresholdTime);
199+
ourLastActivity, localMRPConfig.mActiveThresholdTime, isFirstMessageOnExchange);
198200
}
199201
case Transport::Type::kTcp:
200202
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)