Skip to content

Commit

Permalink
Clarify CASE session establishment API error reporting. (project-chip…
Browse files Browse the repository at this point in the history 
…#18569)

* Clarify CASE session establishment API error reporting.

Consumers were very unclear on the fact that in sync-error cases we
would both return error _and_ call the error callback, leading some of
them to mis-handle the error processing.  For example, the OTA
requestor code would double-notify its consumer on errors.

This PR on its own does not entirely fix that, but makes the API
contract a lot clearer.

Change details:

* The confusingly named Initialized state of OperationalDeviceProxy
  has been renamed to HasAddress.

* The new ResolvingAddress state for OperationalDeviceProxy is to
  allow the "destroy the OperationalDeviceProxy if it's not going to
  be successful" cleanup logic in
  CASESessionManager::FindOrEstablishSession to keep working.

* The change to grab the peer adddress in
  OperationalDeviceProxy::AttachToExistingSecureSession is fixing a
  pre-existing problem, where we could end up not setting the address
  there, then get the session evicted and end up in our "has address,
  need to establish session next" state but without an actual address.

* Various unused APIs that date back to a quite different
  OperationalDeviceProxy setup, with an address cache involved, were
  removed.

* In CASESessionManager::FindOrEstablishSession the out-of-memory case
  was violating the API contract, which allows passing in null
  callbacks; it would crash in that case.

* Fix tv examples.

* Address review comments.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored May 19, 2022
1 parent 2bd12ee commit 1165152
Show file tree
Hide file tree
Showing 12 changed files with 130 additions and 125 deletions.
5 changes: 3 additions & 2 deletions examples/tv-casting-app/tv-casting-common/commands/clusters/ModelCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ CHIP_ERROR ModelCommand::RunCommand()
}

PeerId peerID = fabric->GetPeerIdForNode(mNodeId);
return server->GetCASESessionManager()->FindOrEstablishSession(peerID, &mOnDeviceConnectedCallback,
&mOnDeviceConnectionFailureCallback);
server->GetCASESessionManager()->FindOrEstablishSession(peerID, &mOnDeviceConnectedCallback,
&mOnDeviceConnectionFailureCallback);
return CHIP_NO_ERROR;
}

void ModelCommand::OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device)
Expand Down
8 changes: 1 addition & 7 deletions examples/tv-casting-app/tv-casting-common/src/TargetVideoPlayerInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,7 @@ CHIP_ERROR TargetVideoPlayerInfo::Initialize(NodeId nodeId, FabricIndex fabricIn

PeerId peerID = fabric->GetPeerIdForNode(nodeId);

CHIP_ERROR err =
server->GetCASESessionManager()->FindOrEstablishSession(peerID, &mOnConnectedCallback, &mOnConnectionFailureCallback);
if (err != CHIP_NO_ERROR)
{
ChipLogError(AppServer, "Could not establish a session to the peer");
return err;
}
server->GetCASESessionManager()->FindOrEstablishSession(peerID, &mOnConnectedCallback, &mOnConnectionFailureCallback);

if (mOperationalDeviceProxy == nullptr)
{
Expand Down
24 changes: 15 additions & 9 deletions src/app/CASESessionManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ CHIP_ERROR CASESessionManager::Init(chip::System::Layer * systemLayer, const CAS
return AddressResolve::Resolver::Instance().Init(systemLayer);
}

CHIP_ERROR CASESessionManager::FindOrEstablishSession(PeerId peerId, Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure)
void CASESessionManager::FindOrEstablishSession(PeerId peerId, Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure)
{
ChipLogDetail(CASESessionManager, "FindOrEstablishSession: PeerId = " ChipLogFormatX64 ":" ChipLogFormatX64,
ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()));
Expand All @@ -43,19 +43,25 @@ CHIP_ERROR CASESessionManager::FindOrEstablishSession(PeerId peerId, Callback::C

if (session == nullptr)
{
onFailure->mCall(onFailure->mContext, peerId, CHIP_ERROR_NO_MEMORY);
return CHIP_ERROR_NO_MEMORY;
if (onFailure != nullptr)
{
onFailure->mCall(onFailure->mContext, peerId, CHIP_ERROR_NO_MEMORY);
}
return;
}
}

CHIP_ERROR err = session->Connect(onConnection, onFailure);
if (err != CHIP_NO_ERROR)
session->Connect(onConnection, onFailure);
if (!session->IsConnected() && !session->IsConnecting() && !session->IsResolvingAddress())
{
// Release the peer rather than the pointer in case the failure handler has already released the session.
// This session is not making progress toward anything. It will have
// notified the consumer about the failure already via the provided
// callbacks, if any.
//
// Release the peer rather than the pointer in case the failure handler
// has already released the session.
ReleaseSession(peerId);
}

return err;
}

void CASESessionManager::ReleaseSession(PeerId peerId)
Expand Down
20 changes: 14 additions & 6 deletions src/app/CASESessionManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,21 @@ class CASESessionManager
void Shutdown() {}

/**
* Find an existing session for the given node ID, or trigger a new session request.
* The caller can optionally provide `onConnection` and `onFailure` callback objects. If provided,
* these will be used to inform the caller about successful or failed connection establishment.
* If the connection is already established, the `onConnection` callback will be immediately called.
* Find an existing session for the given node ID, or trigger a new session
* request.
*
* The caller can optionally provide `onConnection` and `onFailure` callback
* objects. If provided, these will be used to inform the caller about
* successful or failed connection establishment.
*
* If the connection is already established, the `onConnection` callback
* will be immediately called, before FindOrEstablishSession returns.
*
* The `onFailure` callback may be called before the FindOrEstablishSession
* call returns, for error cases that are detected synchronously.
*/
CHIP_ERROR FindOrEstablishSession(PeerId peerId, Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure);
void FindOrEstablishSession(PeerId peerId, Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure);

OperationalDeviceProxy * FindExistingSession(PeerId peerId) const;

Expand Down
62 changes: 38 additions & 24 deletions src/app/OperationalDeviceProxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ void OperationalDeviceProxy::MoveToState(State aTargetState)

bool OperationalDeviceProxy::AttachToExistingSecureSession()
{
VerifyOrReturnError(mState == State::NeedsAddress || mState == State::Initialized, false);
VerifyOrReturnError(mState == State::NeedsAddress || mState == State::ResolvingAddress || mState == State::HasAddress, false);

ScopedNodeId peerNodeId(mPeerId.GetNodeId(), mFabricInfo->GetFabricIndex());
auto sessionHandle =
Expand All @@ -74,15 +74,16 @@ bool OperationalDeviceProxy::AttachToExistingSecureSession()
{
ChipLogProgress(Controller, "Found an existing secure session to [" ChipLogFormatX64 "-" ChipLogFormatX64 "]!",
ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId()));
mDeviceAddress = sessionHandle.Value()->AsSecureSession()->GetPeerAddress();
mSecureSession.Grab(sessionHandle.Value());
return true;
}

return false;
}

CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure)
void OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure)
{
CHIP_ERROR err = CHIP_NO_ERROR;
bool isConnected = false;
Expand All @@ -104,12 +105,25 @@ CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected>
isConnected = AttachToExistingSecureSession();
if (!isConnected)
{
// LookupPeerAddress could perhaps call back with a result
// synchronously, so do our state update first.
MoveToState(State::ResolvingAddress);
err = LookupPeerAddress();
if (err != CHIP_NO_ERROR)
{
// Roll back the state change, since we are presumably not in
// the middle of a lookup.
MoveToState(State::NeedsAddress);
}
}

break;

case State::Initialized:
case State::ResolvingAddress:
isConnected = AttachToExistingSecureSession();
break;

case State::HasAddress:
isConnected = AttachToExistingSecureSession();
if (!isConnected)
{
Expand Down Expand Up @@ -143,14 +157,14 @@ CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected>
{
DequeueConnectionCallbacks(err);
}

return err;
}

CHIP_ERROR OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress & addr,
const ReliableMessageProtocolConfig & config)
void OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress & addr, const ReliableMessageProtocolConfig & config)
{
VerifyOrReturnLogError(mState != State::Uninitialized, CHIP_ERROR_INCORRECT_STATE);
if (mState == State::Uninitialized)
{
return;
}

#if CHIP_DETAIL_LOGGING
char peerAddrBuff[Transport::PeerAddress::kMaxToStringSize];
Expand All @@ -171,9 +185,9 @@ CHIP_ERROR OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress
mCASEClient->SetRemoteMRPIntervals(mRemoteMRPConfig);
}

if (mState == State::NeedsAddress)
if (mState == State::ResolvingAddress)
{
MoveToState(State::Initialized);
MoveToState(State::HasAddress);
err = EstablishConnection();
if (err != CHIP_NO_ERROR)
{
Expand All @@ -185,16 +199,14 @@ CHIP_ERROR OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress
if (!mSecureSession)
{
// Nothing needs to be done here. It's not an error to not have a
// secureSession. For one thing, we could have gotten an different
// secureSession. For one thing, we could have gotten a different
// UpdateAddress already and that caused connections to be torn down and
// whatnot.
return CHIP_NO_ERROR;
return;
}

mSecureSession.Get().Value()->AsSecureSession()->SetPeerAddress(addr);
}

return err;
}

CHIP_ERROR OperationalDeviceProxy::EstablishConnection()
Expand Down Expand Up @@ -281,7 +293,7 @@ void OperationalDeviceProxy::OnSessionEstablishmentError(CHIP_ERROR error)
// was just CASE connection failure. So let's re-use the cached address to re-do CASE again
// if need-be.
//
MoveToState(State::Initialized);
MoveToState(State::HasAddress);

DequeueConnectionCallbacks(error);

Expand All @@ -307,17 +319,11 @@ CHIP_ERROR OperationalDeviceProxy::Disconnect()
{
mInitParams.sessionManager->ExpirePairing(mSecureSession.Get().Value());
}
MoveToState(State::Initialized);
MoveToState(State::HasAddress);

return CHIP_NO_ERROR;
}

void OperationalDeviceProxy::Clear()
{
MoveToState(State::Uninitialized);
mInitParams = DeviceProxyInitParams();
}

void OperationalDeviceProxy::CleanupCASEClient()
{
if (mCASEClient)
Expand All @@ -329,7 +335,7 @@ void OperationalDeviceProxy::CleanupCASEClient()

void OperationalDeviceProxy::OnSessionReleased()
{
MoveToState(State::Initialized);
MoveToState(State::HasAddress);
}

CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions()
Expand Down Expand Up @@ -361,6 +367,9 @@ OperationalDeviceProxy::~OperationalDeviceProxy()

CHIP_ERROR OperationalDeviceProxy::LookupPeerAddress()
{
// NOTE: This is public API that can be used to update our stored peer
// address even when we are in State::Connected, so we do not make any
// MoveToState calls in this method.
if (mAddressLookupHandle.IsActive())
{
ChipLogProgress(Discovery, "Operational node lookup already in progress. Will NOT start a new one.");
Expand All @@ -382,6 +391,11 @@ void OperationalDeviceProxy::OnNodeAddressResolutionFailed(const PeerId & peerId
ChipLogError(Discovery, "Operational discovery failed for 0x" ChipLogFormatX64 ": %" CHIP_ERROR_FORMAT,
ChipLogValueX64(peerId.GetNodeId()), reason.Format());

if (IsResolvingAddress())
{
MoveToState(State::NeedsAddress);
}

DequeueConnectionCallbacks(reason);
}

Expand Down
66 changes: 27 additions & 39 deletions src/app/OperationalDeviceProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,6 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
mAddressLookupHandle.SetListener(this);
}

OperationalDeviceProxy(DeviceProxyInitParams & params, PeerId peerId, const Dnssd::ResolvedNodeData & nodeResolutionData) :
OperationalDeviceProxy(params, peerId)
{
mAddressLookupHandle.SetListener(this);
OnNodeIdResolved(nodeResolutionData);
}

void Clear();

/*
* This function can be called to establish a secure session with the device.
*
Expand All @@ -128,17 +119,28 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
* On establishing the session, the callback function `onConnection` will be called. If the
* session setup fails, `onFailure` will be called.
*
* If the session already exists, `onConnection` will be called immediately.
* If the resolver is null and the device state is State::NeedsAddress, CHIP_ERROR_INVALID_ARGUMENT will be
* returned.
* If the session already exists, `onConnection` will be called immediately,
* before the Connect call returns.
*
* `onFailure` may be called before the Connect call returns, for error
* cases that are detected synchronously (e.g. inability to start an address
* lookup).
*/
CHIP_ERROR Connect(Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure);
void Connect(Callback::Callback<OnDeviceConnected> * onConnection, Callback::Callback<OnDeviceConnectionFailure> * onFailure);

bool IsConnected() const { return mState == State::SecureConnected; }

bool IsConnecting() const { return mState == State::Connecting; }

/**
* 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 @@ -149,33 +151,13 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
*/
void OnSessionReleased() override;

void OnNodeIdResolved(const Dnssd::ResolvedNodeData & nodeResolutionData)
{
mDeviceAddress = ToPeerAddress(nodeResolutionData);

mRemoteMRPConfig = nodeResolutionData.resolutionData.GetMRPConfig();

if (mState == State::NeedsAddress)
{
mState = State::Initialized;
}
}

/**
* Mark any open session with the device as expired.
*/
CHIP_ERROR Disconnect() override;

NodeId GetDeviceId() const override { return mPeerId.GetNodeId(); }

/**
* Update data of the device.
* This function will set new IP address, port and MRP retransmission intervals of the device.
* Since the device settings might have been moved from RAM to the persistent storage, the function
* will load the device settings first, before making the changes.
*/
CHIP_ERROR UpdateDeviceData(const Transport::PeerAddress & addr, const ReliableMessageProtocolConfig & config);

PeerId GetPeerId() const { return mPeerId; }

CHIP_ERROR ShutdownSubscriptions() override;
Expand Down Expand Up @@ -227,11 +209,12 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
private:
enum class State
{
Uninitialized,
NeedsAddress,
Initialized,
Connecting,
SecureConnected,
Uninitialized, // Error state: OperationalDeviceProxy is useless
NeedsAddress, // No address known, lookup not started yet.
ResolvingAddress, // Address lookup in progress.
HasAddress, // Have an address, CASE handshake not started yet.
Connecting, // CASE handshake in progress.
SecureConnected, // CASE session established.
};

DeviceProxyInitParams mInitParams;
Expand Down Expand Up @@ -285,6 +268,11 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
*
*/
void DequeueConnectionCallbacks(CHIP_ERROR error);

/**
* This function will set new IP address, port and MRP retransmission intervals of the device.
*/
void UpdateDeviceData(const Transport::PeerAddress & addr, const ReliableMessageProtocolConfig & config);
};

} // namespace chip
Loading

0 comments on commit 1165152

Please sign in to comment.