Skip to content

Commit

Permalink
Make FindOrEstablishSession inside CASE callbacks work. (#27942)
Browse files Browse the repository at this point in the history
The following sequence of events:

1. Someone calls FindOrEstablishSession.
2. When the session establishment fails, OperationalSessionSetup dispatches the
   relevant notifications.
3. One of those notification handlers tries to FindOrEstablishSession to the
   same peer.

failed, because FindOrEstablishSession would pick up the existing
OperationalSessionSetup and add the new callbacks to it... but the existing
OperationalSessionSetup was already in callback notification, so would just
ignore the new callbacks.  So the new FindOrEstablishSession call would never
complete and would never try any actual session establishment.

The fix is to destroy the OperationalSessionSetup before notifying its
callbacks, so a FindOrEstablishSession from one of the callbacks ends up
creating a new OperationalSessionSetup as needed.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Oct 10, 2023
1 parent ab1115b commit 1109955
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 21 deletions.
42 changes: 28 additions & 14 deletions src/app/OperationalSessionSetup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ void OperationalSessionSetup::EnqueueConnectionCallbacks(Callback::Callback<OnDe
}
}

void OperationalSessionSetup::DequeueConnectionCallbacksWithoutReleasing(CHIP_ERROR error)
void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error, ReleaseBehavior releaseBehavior)
{
Cancelable failureReady, successReady;

Expand All @@ -297,50 +297,64 @@ void OperationalSessionSetup::DequeueConnectionCallbacksWithoutReleasing(CHIP_ER
}
#endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES

// Gather up state we will need for our notifications.
bool performingAddressUpdate = mPerformingAddressUpdate;
auto * exchangeMgr = mInitParams.exchangeMgr;
Optional<SessionHandle> optionalSessionHandle = mSecureSession.Get();
ScopedNodeId peerId = mPeerId;

if (releaseBehavior == ReleaseBehavior::Release)
{
VerifyOrDie(mReleaseDelegate != nullptr);
mReleaseDelegate->ReleaseSession(this);
}

// DO NOT touch any members of this object after this point. It's dead.

NotifyConnectionCallbacks(failureReady, successReady, error, peerId, performingAddressUpdate, exchangeMgr,
optionalSessionHandle);
}

void OperationalSessionSetup::NotifyConnectionCallbacks(Cancelable & failureReady, Cancelable & successReady, CHIP_ERROR error,
const ScopedNodeId & peerId, bool performingAddressUpdate,
Messaging::ExchangeManager * exchangeMgr,
const Optional<SessionHandle> & optionalSessionHandle)
{
//
// 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);
VerifyOrDie(!performingAddressUpdate);
Callback::Callback<OnDeviceConnectionFailure> * cb =
Callback::Callback<OnDeviceConnectionFailure>::FromCancelable(failureReady.mNext);

cb->Cancel();

if (error != CHIP_NO_ERROR)
{
cb->mCall(cb->mContext, mPeerId, error);
cb->mCall(cb->mContext, peerId, error);
}
}

while (successReady.mNext != &successReady)
{
// We expect that we only have callbacks if we are not performing just address update.
VerifyOrDie(!mPerformingAddressUpdate);
VerifyOrDie(!performingAddressUpdate);
Callback::Callback<OnDeviceConnected> * cb = Callback::Callback<OnDeviceConnected>::FromCancelable(successReady.mNext);

cb->Cancel();
if (error == CHIP_NO_ERROR)
{
auto * exchangeMgr = mInitParams.exchangeMgr;
VerifyOrDie(exchangeMgr);
// We know that we for sure have the SessionHandle in the successful case.
auto optionalSessionHandle = mSecureSession.Get();
cb->mCall(cb->mContext, *exchangeMgr, optionalSessionHandle.Value());
}
}
}

void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error)
{
DequeueConnectionCallbacksWithoutReleasing(error);
VerifyOrDie(mReleaseDelegate != nullptr);
mReleaseDelegate->ReleaseSession(this);
}

void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error)
{
VerifyOrReturn(mState == State::Connecting,
Expand Down Expand Up @@ -447,7 +461,7 @@ OperationalSessionSetup::~OperationalSessionSetup()
CancelSessionSetupReattempt();
#endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES

DequeueConnectionCallbacksWithoutReleasing(CHIP_ERROR_CANCELLED);
DequeueConnectionCallbacks(CHIP_ERROR_CANCELLED, ReleaseBehavior::DoNotRelease);
}

CHIP_ERROR OperationalSessionSetup::LookupPeerAddress()
Expand Down
26 changes: 19 additions & 7 deletions src/app/OperationalSessionSetup.h
Original file line number Diff line number Diff line change
Expand Up @@ -301,24 +301,36 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate,
void EnqueueConnectionCallbacks(Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure);

enum class ReleaseBehavior
{
Release,
DoNotRelease
};

/*
* This dequeues all failure and success callbacks and appropriately
* invokes either set depending on the value of error.
*
* 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`.
* If releaseBehavior is Release, this uses mReleaseDelegate to release
* ourselves (aka `this`). As a result any caller should return right away
* without touching `this`.
*
* Setting releaseBehavior to DoNotRelease is meant for use from the destructor
*/
void DequeueConnectionCallbacks(CHIP_ERROR error);
void DequeueConnectionCallbacks(CHIP_ERROR error, ReleaseBehavior releaseBehavior = ReleaseBehavior::Release);

/*
* Like DequeueConnectionCallbacks but does not release ourselves. For use
* from our destructor.
/**
* Helper for DequeueConnectionCallbacks that handles the actual callback
* notifications. This happens after the object has been released, if it's
* being released.
*/
void DequeueConnectionCallbacksWithoutReleasing(CHIP_ERROR error);
static void NotifyConnectionCallbacks(Callback::Cancelable & failureReady, Callback::Cancelable & successReady,
CHIP_ERROR error, const ScopedNodeId & peerId, bool performingAddressUpdate,
Messaging::ExchangeManager * exchangeMgr,
const Optional<SessionHandle> & optionalSessionHandle);

/**
* Triggers a DNSSD lookup to find a usable peer address.
Expand Down

0 comments on commit 1109955

Please sign in to comment.