From ee28e16589034396999bcfe89765e7bc52bf74f2 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 15 Nov 2022 21:15:56 -0500 Subject: [PATCH 1/2] Improve logic for deciding whether to re-establish CASE in ReadClient. We could end up in a situation where we had a defunct session but did not have the "try to re-establish CASE" flag set. In that case we would keep trying resubscribe attempts, which would keep failing because we could not actually create an exchange on our session, and we would never recover from that. The fix is that we try to re-establish CASE (assuming the IM engine has a CASE Session manager) if we don't have an active session. Also fixes an incorrect error ("no memory") being reported if we in fact try to subscribe when our session is not active. --- src/app/ReadClient.cpp | 53 ++++++++++++++++++++++++++++++++---------- src/app/ReadClient.h | 2 +- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 94a29cd36dac52..fa050ff0fe845b 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -143,7 +143,7 @@ CHIP_ERROR ReadClient::ScheduleResubscription(uint32_t aTimeTillNextResubscripti mReadPrepareParams.mSessionHolder.Grab(aNewSessionHandle.Value()); } - mDoCaseOnNextResub = aReestablishCASE; + mForceCaseOnNextResub = aReestablishCASE; ReturnErrorOnFailure( InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( @@ -1014,7 +1014,16 @@ CHIP_ERROR ReadClient::SendSubscribeRequestImpl(const ReadPrepareParams & aReadP VerifyOrReturnError(aReadPrepareParams.mSessionHolder, CHIP_ERROR_MISSING_SECURE_SESSION); auto exchange = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get().Value(), this); - VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_NO_MEMORY); + if (exchange == nullptr) + { + if (aReadPrepareParams.mSessionHolder->AsSecureSession()->IsActiveSession()) + { + return CHIP_ERROR_NO_MEMORY; + } + + // Trying to subscribe with a defunct session somehow. + return CHIP_ERROR_INCORRECT_STATE; + } mExchange.Grab(exchange); @@ -1082,14 +1091,11 @@ void ReadClient::OnResubscribeTimerCallback(System::Layer * apSystemLayer, void CHIP_ERROR err; - ChipLogProgress(DataManagement, "OnResubscribeTimerCallback: DoCASE = %d", _this->mDoCaseOnNextResub); + ChipLogProgress(DataManagement, "OnResubscribeTimerCallback: ForceCASE = %d", _this->mForceCaseOnNextResub); _this->mNumRetries++; - if (_this->mDoCaseOnNextResub) + if (_this->mForceCaseOnNextResub) { - auto * caseSessionManager = InteractionModelEngine::GetInstance()->GetCASESessionManager(); - VerifyOrExit(caseSessionManager != nullptr, err = CHIP_ERROR_INCORRECT_STATE); - // // We need to mark our session as defunct explicitly since the assessment of a liveness failure // is usually triggered by the absence of any exchange activity that would have otherwise @@ -1102,10 +1108,33 @@ void ReadClient::OnResubscribeTimerCallback(System::Layer * apSystemLayer, void { _this->mReadPrepareParams.mSessionHolder->AsSecureSession()->MarkAsDefunct(); } + } + + bool allowResubscribeOnError = true; + if (!_this->mReadPrepareParams.mSessionHolder || + !_this->mReadPrepareParams.mSessionHolder->AsSecureSession()->IsActiveSession()) + { + // We don't have an active CASE session. We need to go ahead and set + // one up, if we can. + ChipLogProgress(DataManagement, "Trying to establish a CASE session"); + auto * caseSessionManager = InteractionModelEngine::GetInstance()->GetCASESessionManager(); + if (caseSessionManager) + { + caseSessionManager->FindOrEstablishSession(_this->mPeer, &_this->mOnConnectedCallback, + &_this->mOnConnectionFailureCallback); + return; + } + + if (_this->mForceCaseOnNextResub) + { + // Caller asked us to force CASE but we have no way to do CASE. + // Just stop trying. + allowResubscribeOnError = false; + } - caseSessionManager->FindOrEstablishSession(_this->mPeer, &_this->mOnConnectedCallback, - &_this->mOnConnectionFailureCallback); - return; + // No way to send our subscribe request. + err = CHIP_ERROR_INCORRECT_STATE; + ExitNow(); } err = _this->SendSubscribeRequest(_this->mReadPrepareParams); @@ -1115,11 +1144,11 @@ void ReadClient::OnResubscribeTimerCallback(System::Layer * apSystemLayer, void { // // Call Close (which should trigger re-subscription again) EXCEPT if we got here because we didn't have a valid - // CASESessionManager pointer when mDoCaseOnNextResub was true. + // CASESessionManager pointer when mForceCaseOnNextResub was true. // // In that case, don't permit re-subscription to occur. // - _this->Close(err, err != CHIP_ERROR_INCORRECT_STATE); + _this->Close(err, allowResubscribeOnError); } } diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 3c28561891ef83..59309da4f894b5 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -501,7 +501,7 @@ class ReadClient : public Messaging::ExchangeDelegate InteractionType mInteractionType = InteractionType::Read; Timestamp mEventTimestamp; - bool mDoCaseOnNextResub = true; + bool mForceCaseOnNextResub = true; chip::Callback::Callback mOnConnectedCallback; chip::Callback::Callback mOnConnectionFailureCallback; From f4a16150cf86f6a2258d0fc7bf2280b02e9751d5 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 16 Nov 2022 18:13:19 -0500 Subject: [PATCH 2/2] Address review comments. --- src/app/ReadClient.cpp | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index fa050ff0fe845b..38d9b37f18ab03 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -144,6 +144,13 @@ CHIP_ERROR ReadClient::ScheduleResubscription(uint32_t aTimeTillNextResubscripti } mForceCaseOnNextResub = aReestablishCASE; + if (mForceCaseOnNextResub && mReadPrepareParams.mSessionHolder) + { + // Mark our existing session defunct, so that we will try to + // re-establish it when the timer fires (unless something re-establishes + // before then). + mReadPrepareParams.mSessionHolder->AsSecureSession()->MarkAsDefunct(); + } ReturnErrorOnFailure( InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer( @@ -1094,22 +1101,6 @@ void ReadClient::OnResubscribeTimerCallback(System::Layer * apSystemLayer, void ChipLogProgress(DataManagement, "OnResubscribeTimerCallback: ForceCASE = %d", _this->mForceCaseOnNextResub); _this->mNumRetries++; - if (_this->mForceCaseOnNextResub) - { - // - // We need to mark our session as defunct explicitly since the assessment of a liveness failure - // is usually triggered by the absence of any exchange activity that would have otherwise - // automatically marked the session as defunct on a response timeout. - // - // Doing so will ensure that the subsequent call to FindOrEstablishSession will not bind to - // an existing established session but rather trigger establishing a new one. - // - if (_this->mReadPrepareParams.mSessionHolder) - { - _this->mReadPrepareParams.mSessionHolder->AsSecureSession()->MarkAsDefunct(); - } - } - bool allowResubscribeOnError = true; if (!_this->mReadPrepareParams.mSessionHolder || !_this->mReadPrepareParams.mSessionHolder->AsSecureSession()->IsActiveSession())