Skip to content

Commit

Permalink
Remove methods from DeviceController that are no longer valid (projec…
Browse files Browse the repository at this point in the history 
…t-chip#21886)

With the refactor for OperationalSessionSetup, there are methods
in DeviceController that we can now permanently remove.
  • Loading branch information
@tehampson @isiu-apple
tehampson authored and isiu-apple committed Sep 16, 2022
1 parent 7921fe3 commit 5d547c5
Show file tree
Hide file tree
Showing 16 changed files with 72 additions and 277 deletions.
5 changes: 0 additions & 5 deletions src/app/CASESessionManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 2 additions & 4 deletions src/app/CASESessionManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
Expand Down
63 changes: 12 additions & 51 deletions src/app/OperationalSessionSetup.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand All @@ -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)
Expand All @@ -366,11 +337,6 @@ void OperationalSessionSetup::OnSessionReleased()
MoveToState(State::HasAddress);
}

void OperationalSessionSetup::OnSessionHang()
{
Disconnect();
}

OperationalSessionSetup::~OperationalSessionSetup()
{
if (mAddressLookupHandle.IsActive())
Expand Down Expand Up @@ -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.
}
Expand Down
60 changes: 26 additions & 34 deletions src/app/OperationalSessionSetup.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -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.
*/
Expand Down
2 changes: 0 additions & 2 deletions src/app/clusters/bindings/BindingManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down
12 changes: 0 additions & 12 deletions src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
13 changes: 11 additions & 2 deletions src/app/clusters/ota-requestor/DefaultOTARequestor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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());
Expand Down
1 change: 1 addition & 0 deletions src/app/clusters/ota-requestor/DefaultOTARequestor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading

0 comments on commit 5d547c5

Please sign in to comment.