Remove methods from DeviceController that are no longer valid (#21886)

With the refactor for OperationalSessionSetup, there are methods
in DeviceController that we can now permanently remove.

src/app/CASESessionManager.cpp

@@ -56,11 +56,6 @@ void CASESessionManager::FindOrEstablishSession(const ScopedNodeId & peerId, Cal
  session->Connect(onConnection, onFailure);
 }
 
-void CASESessionManager::ReleaseSession(const ScopedNodeId & peerId)
-{
- ReleaseSession(FindExistingSessionSetup(peerId));
-}
-
 void CASESessionManager::ReleaseSessionsForFabric(FabricIndex fabricIndex)
 {
  mConfig.sessionSetupPool->ReleaseAllSessionSetupsForFabric(fabricIndex);

src/app/CASESessionManager.h

@@ -78,10 +78,6 @@ class CASESessionManager : public OperationalSessionReleaseDelegate, public Sess
  void FindOrEstablishSession(const ScopedNodeId & peerId, Callback::Callback<OnDeviceConnected> * onConnection,
  Callback::Callback<OnDeviceConnectionFailure> * onFailure);
 
- OperationalSessionSetup * FindExistingSessionSetup(const ScopedNodeId & peerId, bool forAddressUpdate = false) const;
-
- void ReleaseSession(const ScopedNodeId & peerId);
-
  void ReleaseSessionsForFabric(FabricIndex fabricIndex);
 
  void ReleaseAllSessions();
@@ -103,6 +99,8 @@ class CASESessionManager : public OperationalSessionReleaseDelegate, public Sess
  void UpdatePeerAddress(ScopedNodeId peerId) override;
 
 private:
+ OperationalSessionSetup * FindExistingSessionSetup(const ScopedNodeId & peerId, bool forAddressUpdate = false) const;
+
  Optional<SessionHandle> FindExistingSession(const ScopedNodeId & peerId) const;
 
  CASESessionManagerConfig mConfig;

src/app/OperationalSessionSetup.cpp

@@ -261,59 +261,50 @@ void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error)
  mConnectionFailure.DequeueAll(failureReady);
  mConnectionSuccess.DequeueAll(successReady);
 
- // TODO Issue #20452: For now we need to make a copy of member fields inside `this` before calling any of the
- // callbacks since the callbacks themselves can potentially release `this` OperationalSessionSetup.
- auto * exchangeMgr = mInitParams.exchangeMgr;
- auto sessionHandle = mSecureSession.Get();
- auto peerId = mPeerId;
- VerifyOrDie(mReleaseDelegate != nullptr);
- auto * releaseDelegate = mReleaseDelegate;
-
  //
  // If we encountered no error, go ahead and call all success callbacks. Otherwise,
  // call the failure callbacks.
  //
  while (failureReady.mNext != &failureReady)
  {
+ // We expect that we only have callbacks if we are not performing just address update.
+ VerifyOrDie(!mPerformingAddressUpdate);
  Callback::Callback<OnDeviceConnectionFailure> * cb =
  Callback::Callback<OnDeviceConnectionFailure>::FromCancelable(failureReady.mNext);
 
  cb->Cancel();
 
  if (error != CHIP_NO_ERROR)
  {
- cb->mCall(cb->mContext, peerId, error);
+ cb->mCall(cb->mContext, mPeerId, error);
  }
  }
 
  while (successReady.mNext != &successReady)
  {
+ // We expect that we only have callbacks if we are not performing just address update.
+ VerifyOrDie(!mPerformingAddressUpdate);
  Callback::Callback<OnDeviceConnected> * cb = Callback::Callback<OnDeviceConnected>::FromCancelable(successReady.mNext);
 
  cb->Cancel();
  if (error == CHIP_NO_ERROR)
  {
- // We know that we for sure have the SessionHandle in the successful case.
+ auto * exchangeMgr = mInitParams.exchangeMgr;
  VerifyOrDie(exchangeMgr);
- cb->mCall(cb->mContext, *exchangeMgr, sessionHandle.Value());
+ // We know that we for sure have the SessionHandle in the successful case.
+ auto optionalSessionHandle = mSecureSession.Get();
+ cb->mCall(cb->mContext, *exchangeMgr, optionalSessionHandle.Value());
  }
  }
-
- releaseDelegate->ReleaseSession(this);
+ VerifyOrDie(mReleaseDelegate != nullptr);
+ mReleaseDelegate->ReleaseSession(this);
 }
 
 void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error)
 {
  VerifyOrReturn(mState != State::Uninitialized && mState != State::NeedsAddress,
  ChipLogError(Controller, "HandleCASEConnectionFailure was called while the device was not initialized"));
 
- //
- // We don't need to reset the state all the way back to NeedsAddress since all that transpired
- // was just CASE connection failure. So let's re-use the cached address to re-do CASE again
- // if need-be.
- //
- MoveToState(State::HasAddress);
-
  DequeueConnectionCallbacks(error);
  // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks.
 }
@@ -332,26 +323,6 @@ void OperationalSessionSetup::OnSessionEstablished(const SessionHandle & session
  // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks.
 }
 
-void OperationalSessionSetup::Disconnect()
-{
- VerifyOrReturn(mState == State::SecureConnected);
-
- if (mSecureSession)
- {
- //
- // Mark the session as defunct to signal that we no longer want to use this
- // session anymore for further interactions to this peer. However, if we receive
- // messages back from that peer on the defunct session, it will bring it back into an active
- // state again.
- //
- mSecureSession.Get().Value()->AsSecureSession()->MarkAsDefunct();
- }
-
- mSecureSession.Release();
-
- MoveToState(State::HasAddress);
-}
-
 void OperationalSessionSetup::CleanupCASEClient()
 {
  if (mCASEClient)
@@ -366,11 +337,6 @@ void OperationalSessionSetup::OnSessionReleased()
  MoveToState(State::HasAddress);
 }
 
-void OperationalSessionSetup::OnSessionHang()
-{
- Disconnect();
-}
-
 OperationalSessionSetup::~OperationalSessionSetup()
 {
  if (mAddressLookupHandle.IsActive())
@@ -456,12 +422,7 @@ void OperationalSessionSetup::OnNodeAddressResolutionFailed(const PeerId & peerI
  ChipLogError(Discovery, "OperationalSessionSetup[%u:" ChipLogFormatX64 "]: operational discovery failed: %" CHIP_ERROR_FORMAT,
  mPeerId.GetFabricIndex(), ChipLogValueX64(mPeerId.GetNodeId()), reason.Format());
 
- if (IsResolvingAddress())
- {
- MoveToState(State::NeedsAddress);
- }
-
- // No need to set mPerformingAddressUpdate to false since call below releases `this`.
+ // No need to modify any variables in `this` since call below releases `this`.
  DequeueConnectionCallbacks(reason);
  // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks.
 }

src/app/OperationalSessionSetup.h

@@ -140,19 +140,29 @@ class OperationalDeviceProxy : public DeviceProxy
  * application code does incorrectly hold onto this information so do not follow those incorrect
  * implementations as an example.
  */
-// TODO: OnDeviceConnected should not return ExchangeManager. Application should have this already. This
-// was provided initially to keep code churn down during a large refactor of OnDeviceConnected.
 typedef void (*OnDeviceConnected)(void * context, Messaging::ExchangeManager & exchangeMgr, SessionHandle & sessionHandle);
 typedef void (*OnDeviceConnectionFailure)(void * context, const ScopedNodeId & peerId, CHIP_ERROR error);
 
 /**
- * Represents a connection path to a device that is in an operational state.
+ * Object used to either establish a connection to peer or performing address lookup to a peer.
  *
- * Handles the lifetime of communicating with such a device:
- * - Discover the device using DNSSD (find out what IP address to use and what
- * communication parameters are appropriate for it)
- * - Establish a secure channel to it via CASE
- * - Expose to consumers the secure session for talking to the device.
+ * OperationalSessionSetup is capable of either:
+ * 1. Establishing a CASE session connection to a peer. Upon success or failure, the OnDeviceConnected or
+ * OnDeviceConnectionFailure callback will be called to notify the caller the results of trying to
+ * estblish a CASE session. Some additional details about the steps to establish a connection are:
+ * - Discover the device using DNSSD (find out what IP address to use and what
+ * communication parameters are appropriate for it)
+ * - Establish a secure channel to it via CASE
+ * - Expose to consumers the secure session for talking to the device via OnDeviceConnected
+ * callback.
+ * 2. Performing an address lookup for given a scoped nodeid. On success, it will call into
+ * SessionManager to update the addresses for all matching sessions in the session table.
+ *
+ * OperationalSessionSetup has a very limited lifetime. Once it has completed its purpose outlined above,
+ * it will use `releaseDelegate` to release itself.
+ *
+ * It is possible to determine which of the two purposes the OperationalSessionSetup is for by calling
+ * IsForAddressUpdate().
  */
 class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
  public SessionEstablishmentDelegate,
@@ -198,21 +208,10 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
  */
  void Connect(Callback::Callback<OnDeviceConnected> * onConnection, Callback::Callback<OnDeviceConnectionFailure> * onFailure);
 
- bool IsConnected() const { return mState == State::SecureConnected; }
-
  bool IsConnecting() const { return mState == State::Connecting; }
 
  bool IsForAddressUpdate() const { return mPerformingAddressUpdate; }
 
- /**
- * IsResolvingAddress returns true if we are doing an address resolution
- * that needs to happen before we can establish CASE. We can be in the
- * middle of doing address updates at other times too (e.g. when we are
- * IsConnected()), but those will not cause a true return from
- * IsResolvingAddress().
- */
- bool IsResolvingAddress() const { return mState == State::ResolvingAddress; }
-
  //////////// SessionEstablishmentDelegate Implementation ///////////////
  void OnSessionEstablished(const SessionHandle & session) override;
  void OnSessionEstablishmentError(CHIP_ERROR error) override;
@@ -221,19 +220,9 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
 
  // Called when a connection is closing. The object releases all resources associated with the connection.
  void OnSessionReleased() 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.
- */
- void Disconnect();
 
  ScopedNodeId GetPeerId() const { return mPeerId; }
 
- Transport::PeerAddress GetPeerAddress() const { return mDeviceAddress; }
-
  static Transport::PeerAddress ToPeerAddress(const Dnssd::ResolvedNodeData & nodeData)
  {
  Inet::InterfaceId interfaceId = Inet::InterfaceId::Null();
@@ -256,11 +245,6 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
  */
  FabricIndex GetFabricIndex() const { return mPeerId.GetFabricIndex(); }
 
- /**
- * Triggers a DNSSD lookup to find a usable peer address for this operational device.
- */
- CHIP_ERROR LookupPeerAddress();
-
  void PerformAddressUpdate();
 
  // AddressResolve::NodeListener - notifications when dnssd finds a node IP address
@@ -331,9 +315,17 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
  * If error == CHIP_NO_ERROR, only success callbacks are invoked.
  * Otherwise, only failure callbacks are invoked.
  *
+ * This uses mReleaseDelegate to release ourselves (aka `this`). As a
+ * result any caller should return right away without touching `this`.
+ *
  */
  void DequeueConnectionCallbacks(CHIP_ERROR error);
 
+ /**
+ * Triggers a DNSSD lookup to find a usable peer address.
+ */
+ CHIP_ERROR LookupPeerAddress();
+
  /**
  * This function will set new IP address, port and MRP retransmission intervals of the device.
  */

src/app/clusters/bindings/BindingManager.cpp

@@ -111,7 +111,6 @@ CHIP_ERROR BindingManager::EstablishConnection(const ScopedNodeId & nodeId)
  if (mPendingNotificationMap.FindLRUConnectPeer(peerToRemove) == CHIP_NO_ERROR)
  {
  mPendingNotificationMap.RemoveAllEntriesForNode(peerToRemove);
- mInitParams.mCASESessionManager->ReleaseSession(peerToRemove);
 
  // Now retry
  mLastSessionEstablishmentError = CHIP_NO_ERROR;
@@ -160,7 +159,6 @@ void BindingManager::HandleDeviceConnectionFailure(const ScopedNodeId & peerId,
 {
  // Simply release the entry, the connection will be re-established as needed.
  ChipLogError(AppServer, "Failed to establish connection to node 0x" ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId()));
- mInitParams.mCASESessionManager->ReleaseSession(peerId);
  mLastSessionEstablishmentError = error;
 }
 

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -296,18 +296,6 @@ void FailSafeCleanup(const chip::DeviceLayer::ChipDeviceEvent * event)
  // Session Context at the Server.
  if (event->FailSafeTimerExpired.addNocCommandHasBeenInvoked || event->FailSafeTimerExpired.updateNocCommandHasBeenInvoked)
  {
- // TODO(#19259): The following scope will no longer need to exist after #19259 is fixed
- {
- CASESessionManager * caseSessionManager = Server::GetInstance().GetCASESessionManager();
- if (caseSessionManager)
- {
- const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
- VerifyOrReturn(fabricInfo != nullptr);
-
- caseSessionManager->ReleaseSessionsForFabric(fabricInfo->GetFabricIndex());
- }
- }
-
  SessionManager & sessionMgr = Server::GetInstance().GetSecureSessionManager();
  CleanupSessionsForFabric(sessionMgr, fabricIndex);
  }

src/app/clusters/ota-requestor/DefaultOTARequestor.cpp

@@ -386,8 +386,15 @@ void DefaultOTARequestor::DisconnectFromProvider()
  return;
  }
 
- auto providerNodeId = GetProviderScopedId();
- mServer->GetSecureSessionManager().MarkSessionsAsDefunct(providerNodeId, MakeOptional(Transport::SecureSession::Type::kCASE));
+ auto optionalSessionHandle = mSessionHolder.Get();
+ if (optionalSessionHandle.HasValue())
+ {
+ if (optionalSessionHandle.Value()->IsActiveSession())
+ {
+ optionalSessionHandle.Value()->AsSecureSession()->MarkAsDefunct();
+ }
+ }
+ mSessionHolder.Release();
 }
 
 // Requestor is directed to cancel image update in progress. All the Requestor state is
@@ -419,6 +426,7 @@ void DefaultOTARequestor::OnConnected(void * context, Messaging::ExchangeManager
 {
  DefaultOTARequestor * requestorCore = static_cast<DefaultOTARequestor *>(context);
  VerifyOrDie(requestorCore != nullptr);
+ requestorCore->mSessionHolder.Grab(sessionHandle);
 
  switch (requestorCore->mOnConnectedAction)
  {
@@ -476,6 +484,7 @@ void DefaultOTARequestor::OnConnectionFailure(void * context, const ScopedNodeId
 {
  DefaultOTARequestor * requestorCore = static_cast<DefaultOTARequestor *>(context);
  VerifyOrDie(requestorCore != nullptr);
+ requestorCore->mSessionHolder.Release();
 
  ChipLogError(SoftwareUpdate, "Failed to connect to node 0x" ChipLogFormatX64 ": %" CHIP_ERROR_FORMAT,
  ChipLogValueX64(peerId.GetNodeId()), error.Format());

src/app/clusters/ota-requestor/DefaultOTARequestor.h

@@ -331,6 +331,7 @@ class DefaultOTARequestor : public OTARequestorInterface, public BDXDownloader::
  // persistent storage (if available), used for sending the NotifyApplied message, and then cleared. This will ensure determinism
  // in the OTARequestorDriver on reboot.
  Optional<ProviderLocationType> mProviderLocation;
+ SessionHolder mSessionHolder;
 };
 
 } // namespace chip