From 2c4bb2853b1d33c542a08d23d527e469045e4047 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 13 Jul 2023 12:29:56 -0400 Subject: [PATCH] Make FindOrEstablishSession inside CASE callbacks work. 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. --- src/app/OperationalSessionSetup.cpp | 42 +++++++++++++++++++---------- src/app/OperationalSessionSetup.h | 26 +++++++++++++----- 2 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp index 849490b40ab8c8..7b5d70919f2a8a 100644 --- a/src/app/OperationalSessionSetup.cpp +++ b/src/app/OperationalSessionSetup.cpp @@ -276,7 +276,7 @@ void OperationalSessionSetup::EnqueueConnectionCallbacks(Callback::Callback 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 & optionalSessionHandle) +{ // // If we encountered no error, go ahead and call all success callbacks. Otherwise, // call the failure callbacks. @@ -304,7 +327,7 @@ void OperationalSessionSetup::DequeueConnectionCallbacksWithoutReleasing(CHIP_ER 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 * cb = Callback::Callback::FromCancelable(failureReady.mNext); @@ -312,35 +335,26 @@ void OperationalSessionSetup::DequeueConnectionCallbacksWithoutReleasing(CHIP_ER 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 * cb = Callback::Callback::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, @@ -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() diff --git a/src/app/OperationalSessionSetup.h b/src/app/OperationalSessionSetup.h index c6208a00f4ba35..bfa76fbc41d730 100644 --- a/src/app/OperationalSessionSetup.h +++ b/src/app/OperationalSessionSetup.h @@ -301,6 +301,12 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate, void EnqueueConnectionCallbacks(Callback::Callback * onConnection, Callback::Callback * onFailure); + enum class ReleaseBehavior + { + Release, + DoNotRelease + }; + /* * This dequeues all failure and success callbacks and appropriately * invokes either set depending on the value of error. @@ -308,17 +314,23 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate, * 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 & optionalSessionHandle); /** * Triggers a DNSSD lookup to find a usable peer address.