Remove SessionRecoveryDelegate, deliver its event via SessionHolder (#…

…18878)

* Remove SessionRecoveryDelegate, deliver its event via SessionHolder

* Resolve comments

src/app/OperationalDeviceProxy.cpp

@@ -340,6 +340,16 @@ void OperationalDeviceProxy::OnSessionReleased()
     MoveToState(State::HasAddress);
 }
 
+void OperationalDeviceProxy::OnFirstMessageDeliveryFailed()
+{
+    LookupPeerAddress();
+}
+
+void OperationalDeviceProxy::OnSessionHang()
+{
+    // TODO: establish a new session
+}
+
 CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions()
 {
     return app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricInfo->GetFabricIndex(), GetDeviceId());

src/app/OperationalDeviceProxy.h

@@ -145,11 +145,15 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
     void OnSessionEstablished(const SessionHandle & session) override;
     void OnSessionEstablishmentError(CHIP_ERROR error) override;
 
-    /**
-     *   Called when a connection is closing.
-     *   The object releases all resources associated with the connection.
-     */
+    //////////// SessionDelegate Implementation ///////////////
+
+    // Called when a connection is closing. The object releases all resources associated with the connection.
     void OnSessionReleased() override;
+    // Called when a message is not acked within first retrans timer, try to refresh the peer address
+    void OnFirstMessageDeliveryFailed() override;
+    // Called when a connection is hanging. Try to re-establish another session, and shift to the new session when done, the
+    // original session won't be touched during the period.
+    void OnSessionHang() override;
 
     /**
      *  Mark any open session with the device as expired.

src/controller/CHIPDeviceController.cpp

@@ -282,44 +282,6 @@ CHIP_ERROR DeviceController::DisconnectDevice(NodeId nodeId)
     return CHIP_NO_ERROR;
 }
 
-void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & session)
-{
-    if (session->GetSessionType() != Session::SessionType::kSecure)
-    {
-        // Definitely not a CASE session.
-        return;
-    }
-
-    auto * secureSession = session->AsSecureSession();
-    if (secureSession->GetSecureSessionType() != SecureSession::Type::kCASE)
-    {
-        // Still not CASE.
-        return;
-    }
-
-    FabricIndex ourIndex = kUndefinedFabricIndex;
-    CHIP_ERROR err       = GetFabricIndex(&ourIndex);
-    if (err != CHIP_NO_ERROR)
-    {
-        // We can't really do CASE, now can we?
-        return;
-    }
-
-    if (ourIndex != session->GetFabricIndex())
-    {
-        // Not one of our sessions.
-        return;
-    }
-
-    err = UpdateDevice(secureSession->GetPeerNodeId());
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(Controller,
-                     "OnFirstMessageDeliveryFailed was called, but UpdateDevice did not succeed (%" CHIP_ERROR_FORMAT ")",
-                     err.Format());
-    }
-}
-
 CHIP_ERROR DeviceController::GetPeerAddressAndPort(PeerId peerId, Inet::IPAddress & addr, uint16_t & port)
 {
     VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
@@ -360,8 +322,6 @@ CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)
 {
     ReturnErrorOnFailure(DeviceController::Init(params));
 
-    params.systemState->SessionMgr()->RegisterRecoveryDelegate(*this);
-
     mPairingDelegate = params.pairingDelegate;
 
     // Configure device attestation validation
@@ -434,8 +394,6 @@ CHIP_ERROR DeviceCommissioner::Shutdown()
     }
     // TODO: If we have a commissioning step in progress, is there a way to cancel that callback?
 
-    mSystemState->SessionMgr()->UnregisterRecoveryDelegate(*this);
-
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
     if (mUdcTransportMgr != nullptr)
     {

src/controller/CHIPDeviceController.h

@@ -136,7 +136,7 @@ struct CommissionerInitParams : public ControllerInitParams
  *   and device pairing information for individual devices). Alternatively, this class can retrieve the
  *   relevant information when the application tries to communicate with the device
  */
-class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public AbstractDnssdDiscoveryController
+class DLL_EXPORT DeviceController : public AbstractDnssdDiscoveryController
 {
 public:
     DeviceController();
@@ -301,9 +301,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
     /// in case subclasses want to create the session if it does not yet exist
     virtual OperationalDeviceProxy * GetDeviceSession(const PeerId & peerId);
 
-    //////////// SessionRecoveryDelegate Implementation ///////////////
-    void OnFirstMessageDeliveryFailed(const SessionHandle & session) override;
-
     DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mCommissionableNodes); }
 
 private:

src/lib/core/CHIPConfig.h

@@ -1053,17 +1053,6 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
     "Please enable at least one of CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_FAST_COPY_SUPPORT or CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_FLEXIBLE_COPY_SUPPORT"
 #endif
 
-/**
- * @def CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES
- *
- * @brief Defines the max number of SessionRecoveryDelegate
- *
- * // TODO: Explain what this is for.
- */
-#ifndef CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES
-#define CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES 4
-#endif
-
 /**
  * @def CHIP_CONFIG_CASE_SESSION_RESUME_CACHE_SIZE
  *

src/messaging/ReliableMessageMgr.cpp

@@ -132,6 +132,10 @@ void ReliableMessageMgr::ExecuteActions()
                          " sendCount: %u max retries: %d",
                          messageCounter, ChipLogValueExchange(&entry->ec.Get()), sendCount, CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS);
 
+            // Don't check whether the session in the exchange is valid, because when the session is released, the retrans entry is
+            // cleared inside ExchangeContext::OnSessionReleased, so the session must be valid if the entry exists.
+            entry->ec->GetSessionHandle()->DispatchSessionEvent(&SessionDelegate::OnSessionHang);
+
             // Do not StartTimer, we will schedule the timer at the end of the timer handler.
             mRetransTable.ReleaseObject(entry);
             return Loop::Continue;
@@ -280,7 +284,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry)
             // After the first failure notify session manager to refresh device data
             if (entry->sendCount == 1)
             {
-                exchangeMgr->GetSessionManager()->RefreshSessionOperationalData(entry->ec->GetSessionHandle());
+                entry->ec->GetSessionHandle()->DispatchSessionEvent(&SessionDelegate::OnFirstMessageDeliveryFailed);
             }
         }
     }

src/messaging/ReliableMessageMgr.h

@@ -38,9 +38,6 @@
 namespace chip {
 namespace Messaging {
 
-class ExchangeContext;
-using ExchangeHandle = ReferenceCountedHandle<ExchangeContext>;
-
 enum class SendMessageFlags : uint16_t;
 class ReliableMessageContext;
 

src/transport/Session.h

@@ -92,6 +92,14 @@ class Session
 
     bool IsSecureSession() const { return GetSessionType() == SessionType::kSecure; }
 
+    void DispatchSessionEvent(SessionDelegate::Event event)
+    {
+        for (auto & holder : mHolders)
+        {
+            holder.DispatchSessionEvent(event);
+        }
+    }
+
 protected:
     // This should be called by sub-classes at the very beginning of the destructor, before any data field is disposed, such that
     // the session is still functional during the callback.

src/transport/SessionDelegate.h

@@ -41,27 +41,29 @@ class DLL_EXPORT SessionDelegate
      */
     virtual NewSessionHandlingPolicy GetNewSessionHandlingPolicy() { return NewSessionHandlingPolicy::kShiftToNewSession; }
 
+    using Event = void (SessionDelegate::*)();
+
     /**
      * @brief
      *   Called when a session is releasing
      */
     virtual void OnSessionReleased() = 0;
-};
 
-class DLL_EXPORT SessionRecoveryDelegate
-{
-public:
-    virtual ~SessionRecoveryDelegate() {}
+    /**
+     * @brief
+     *   Called when the first message delivery in an exchange fails, so actions aiming to recover connection can be performed.
+     *
+     *   Note: the implementation must not do anything that will destroy the session or change the SessionHolder.
+     */
+    virtual void OnFirstMessageDeliveryFailed() {}
 
     /**
      * @brief
-     *   Called when the first message delivery in a session failed,
-     *   so actions aiming to recover connection can be performed.
+     *   Called when a session is unresponsive for a while (detected by MRP)
      *
-     * @param session   The handle to the session.  This may be any session type
-     *                  that supports MRP.
+     *   Note: the implementation must not do anything that will destroy the session or change the SessionHolder.
      */
-    virtual void OnFirstMessageDeliveryFailed(const SessionHandle & session) = 0;
+    virtual void OnSessionHang() {}
 };
 
 } // namespace chip

src/transport/SessionHolder.h

@@ -66,11 +66,14 @@ class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase
 
     Transport::Session * operator->() const { return &mSession.Value().Get(); }
 
+    // There is not delegate, nothing to do here
+    virtual void DispatchSessionEvent(SessionDelegate::Event event) {}
+
 private:
     Optional<ReferenceCountedHandle<Transport::Session>> mSession;
 };
 
-// @brief Extends SessionHolder to allow propagate OnSessionReleased event to an extra given destination
+/// @brief Extends SessionHolder to allow propagate SessionDelegate::* events to a given destination
 class SessionHolderWithDelegate : public SessionHolder
 {
 public:
@@ -86,6 +89,8 @@ class SessionHolderWithDelegate : public SessionHolder
         mDelegate.OnSessionReleased();
     }
 
+    void DispatchSessionEvent(SessionDelegate::Event event) override { (mDelegate.*event)(); }
+
 private:
     SessionDelegate & mDelegate;
 };

src/transport/SessionManager.cpp

@@ -102,8 +102,6 @@ CHIP_ERROR SessionManager::Init(System::Layer * systemLayer, TransportMgrBase *
 
 void SessionManager::Shutdown()
 {
-    mSessionRecoveryDelegates.ReleaseAll();
-
     mMessageCounterManager = nullptr;
 
     mState        = State::kNotReady;
@@ -435,38 +433,6 @@ void SessionManager::OnMessageReceived(const PeerAddress & peerAddress, System::
     }
 }
 
-void SessionManager::RegisterRecoveryDelegate(SessionRecoveryDelegate & cb)
-{
-#ifndef NDEBUG
-    mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionRecoveryDelegate> * i) {
-        VerifyOrDie(std::addressof(cb) != std::addressof(i->get()));
-        return Loop::Continue;
-    });
-#endif
-    std::reference_wrapper<SessionRecoveryDelegate> * slot = mSessionRecoveryDelegates.CreateObject(cb);
-    VerifyOrDie(slot != nullptr);
-}
-
-void SessionManager::UnregisterRecoveryDelegate(SessionRecoveryDelegate & cb)
-{
-    mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionRecoveryDelegate> * i) {
-        if (std::addressof(cb) == std::addressof(i->get()))
-        {
-            mSessionRecoveryDelegates.ReleaseObject(i);
-            return Loop::Break;
-        }
-        return Loop::Continue;
-    });
-}
-
-void SessionManager::RefreshSessionOperationalData(const SessionHandle & sessionHandle)
-{
-    mSessionRecoveryDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionRecoveryDelegate> * cb) {
-        cb->get().OnFirstMessageDeliveryFailed(sessionHandle);
-        return Loop::Continue;
-    });
-}
-
 void SessionManager::UnauthenticatedMessageDispatch(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress,
                                                     System::PacketBufferHandle && msg)
 {

src/transport/SessionManager.h

@@ -150,10 +150,6 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
     /// ExchangeManager)
     void SetMessageDelegate(SessionMessageDelegate * cb) { mCB = cb; }
 
-    void RegisterRecoveryDelegate(SessionRecoveryDelegate & cb);
-    void UnregisterRecoveryDelegate(SessionRecoveryDelegate & cb);
-    void RefreshSessionOperationalData(const SessionHandle & sessionHandle);
-
     // Test-only: create a session on the fly.
     CHIP_ERROR InjectPaseSessionWithTestKey(SessionHolder & sessionHolder, uint16_t localSessionId, NodeId peerNodeId,
                                             uint16_t peerSessionId, FabricIndex fabricIndex,
@@ -267,9 +263,6 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
 
     SessionMessageDelegate * mCB = nullptr;
 
-    ObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
-        mSessionRecoveryDelegates;
-
     TransportMgrBase * mTransportMgr                                   = nullptr;
     Transport::MessageCounterManagerInterface * mMessageCounterManager = nullptr;