From 711282053340dd7dc191e6502519bbf19688e02a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 16 Mar 2022 17:41:25 -0400 Subject: [PATCH] Add a AddressResolve::Resolver Shutdown command. (#16239) * Add a AddressResolve::Resolver Shutdown command. Ensures that we can cleanly cancel any active resolves in case tools/commands need to shut down early. * Also clear out the operational delegate during shutdown * Added support for active lookup cancellation and use that in the resolver proxy * Update the comment on Cancel to make it clear that no argument defaults was an intentional decision and somewhat discourage any future changes that may find it convenient to default to skip or cancel * Add TODO about cancelling dnssd lookups and also added a timer adjustement on lookup cancellation * Code review updates --- src/app/OperationalDeviceProxy.cpp | 24 +++++++-- src/controller/CHIPDeviceController.h | 2 - src/lib/address_resolve/AddressResolve.h | 26 ++++++++++ .../AddressResolve_DefaultImpl.cpp | 50 +++++++++++++++++++ .../AddressResolve_DefaultImpl.h | 2 + src/lib/core/CHIPError.h | 18 +++++-- 6 files changed, 113 insertions(+), 9 deletions(-) diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 2ecd1287def89f..95edcadc4ec0fe 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -40,6 +40,9 @@ #include using namespace chip::Callback; +using chip::AddressResolve::NodeLookupRequest; +using chip::AddressResolve::Resolver; +using chip::AddressResolve::ResolveResult; namespace chip { @@ -309,6 +312,19 @@ CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions() OperationalDeviceProxy::~OperationalDeviceProxy() { + if (mAddressLookupHandle.IsActive()) + { + ChipLogProgress(Discovery, "Cancelling incomplete address resolution as device is being deleted."); + + // Skip cancel callback since the destructor is being called, so we assume that this object is + // obviously not used anymore + CHIP_ERROR err = Resolver::Instance().CancelLookup(mAddressLookupHandle, Resolver::FailureCallback::Skip); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Discovery, "Lookup cancel failed: %" CHIP_ERROR_FORMAT, err.Format()); + } + } + if (mCASEClient) { // Make sure we don't leak it. @@ -318,18 +334,18 @@ OperationalDeviceProxy::~OperationalDeviceProxy() CHIP_ERROR OperationalDeviceProxy::LookupPeerAddress() { - if (mAddressLookupHandle.IsInList()) + if (mAddressLookupHandle.IsActive()) { ChipLogProgress(Discovery, "Operational node lookup already in progress. Will NOT start a new one."); return CHIP_NO_ERROR; } - AddressResolve::NodeLookupRequest request(mPeerId); + NodeLookupRequest request(mPeerId); - return AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle); + return Resolver::Instance().LookupNode(request, mAddressLookupHandle); } -void OperationalDeviceProxy::OnNodeAddressResolved(const PeerId & peerId, const AddressResolve::ResolveResult & result) +void OperationalDeviceProxy::OnNodeAddressResolved(const PeerId & peerId, const ResolveResult & result) { UpdateDeviceData(result.address, result.mrpConfig); } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index ef7160d6c7a859..fccaa090af7a08 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -71,8 +71,6 @@ #include #endif #include -#include -#include namespace chip { diff --git a/src/lib/address_resolve/AddressResolve.h b/src/lib/address_resolve/AddressResolve.h index 4a30b8876ecf91..4b2eef1301b1a3 100644 --- a/src/lib/address_resolve/AddressResolve.h +++ b/src/lib/address_resolve/AddressResolve.h @@ -81,6 +81,9 @@ class NodeLookupHandleBase : public IntrusiveListNodeBase void SetListener(NodeListener * listener) { mListener = listener; } NodeListener * GetListener() { return mListener; } + /// Convenience method that is more readable than 'IsInList' + inline bool IsActive() const { return IntrusiveListNodeBase::IsInList(); } + protected: NodeListener * mListener = nullptr; }; @@ -168,6 +171,14 @@ class NodeLookupHandle; class Resolver { public: + /// Enumeration defining how to handle cancel callbacks during operation + /// cancellation. + enum class FailureCallback + { + Call, // Call the failure callback + Skip // do not call the failure callback (generally silent operation) + }; + virtual ~Resolver(); /// Expected to be called at least once before the resolver is ever @@ -193,6 +204,21 @@ class Resolver /// in progress) virtual CHIP_ERROR LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) = 0; + /// Stops an active lookup request. + /// + /// Caller controlls weather the `fail` callback of the handle is invoked or not by using + /// the `cancel_method` argument. + /// + /// Note that there is no default cancel_method on purpose, so that the caller has to make + /// a clear decision if the callback should or should not be invoked. + virtual CHIP_ERROR CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method) = 0; + + /// Shut down any active resolves + /// + /// Will immediately fail any scheduled resolve calls and will refuse to register + /// any new lookups until re-initialized. + virtual void Shutdown() = 0; + /// Expected to be provided by the implementation. static Resolver & Instance(); }; diff --git a/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp b/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp index ad410e4f9dea6d..848b31b61b792b 100644 --- a/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp +++ b/src/lib/address_resolve/AddressResolve_DefaultImpl.cpp @@ -181,6 +181,8 @@ NodeLookupAction NodeLookupHandle::NextAction(System::Clock::Timestamp now) CHIP_ERROR Resolver::LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) { + VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE); + handle.ResetForLookup(mTimeSource.GetMonotonicTimestamp(), request); ReturnErrorOnFailure(Dnssd::Resolver::Instance().ResolveNodeId(request.GetPeerId(), Inet::IPAddressType::kAny)); mActiveLookups.PushBack(&handle); @@ -188,6 +190,29 @@ CHIP_ERROR Resolver::LookupNode(const NodeLookupRequest & request, Impl::NodeLoo return CHIP_NO_ERROR; } +CHIP_ERROR Resolver::CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method) +{ + VerifyOrReturnError(handle.IsActive(), CHIP_ERROR_INVALID_ARGUMENT); + mActiveLookups.Remove(&handle); + + // Adjust any timing updates. + ReArmTimer(); + + if (cancel_method == FailureCallback::Call) + { + handle.GetListener()->OnNodeAddressResolutionFailed(handle.GetRequest().GetPeerId(), CHIP_ERROR_CANCELLED); + } + + // TODO: There should be some form of cancel into Dnssd::Resolver::Instance() + // to stop any resolution mechanism if applicable. + // + // Current code just removes the internal list and any callbacks of resolution will + // be ignored. This works from the perspective of the caller of this method, + // but may be wasteful by letting dnssd still work in the background. + + return CHIP_NO_ERROR; +} + CHIP_ERROR Resolver::Init(System::Layer * systemLayer) { mSystemLayer = systemLayer; @@ -195,6 +220,31 @@ CHIP_ERROR Resolver::Init(System::Layer * systemLayer) return CHIP_NO_ERROR; } +void Resolver::Shutdown() +{ + while (mActiveLookups.begin() != mActiveLookups.end()) + { + auto current = mActiveLookups.begin(); + + const PeerId peerId = current->GetRequest().GetPeerId(); + NodeListener * listener = current->GetListener(); + + mActiveLookups.Erase(current); + + // Failure callback only called after iterator was cleared: + // This allows failure handlers to deallocate structures that may + // contain the active lookup data as a member (intrusive lists members) + listener->OnNodeAddressResolutionFailed(peerId, CHIP_ERROR_SHUT_DOWN); + } + + // Re-arm of timer is expected to cancel any active timer as the + // internal list of active lookups is empty at this point. + ReArmTimer(); + + mSystemLayer = nullptr; + Dnssd::Resolver::Instance().SetOperationalDelegate(nullptr); +} + void Resolver::OnOperationalNodeResolved(const Dnssd::ResolvedNodeData & nodeData) { auto it = mActiveLookups.begin(); diff --git a/src/lib/address_resolve/AddressResolve_DefaultImpl.h b/src/lib/address_resolve/AddressResolve_DefaultImpl.h index 57d6e14c812d67..ab234b88ecad69 100644 --- a/src/lib/address_resolve/AddressResolve_DefaultImpl.h +++ b/src/lib/address_resolve/AddressResolve_DefaultImpl.h @@ -135,6 +135,8 @@ class Resolver : public ::chip::AddressResolve::Resolver, public Dnssd::Operatio CHIP_ERROR Init(System::Layer * systemLayer) override; CHIP_ERROR LookupNode(const NodeLookupRequest & request, Impl::NodeLookupHandle & handle) override; + CHIP_ERROR CancelLookup(Impl::NodeLookupHandle & handle, FailureCallback cancel_method) override; + void Shutdown() override; // Dnssd::OperationalResolveDelegate diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index f8c3458caade7b..d1f7c440c7312d 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -1466,8 +1466,21 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_TOO_MANY_CONNECTIONS CHIP_CORE_ERROR(0x72) -// unused CHIP_CORE_ERROR(0x73) -// unused CHIP_CORE_ERROR(0x74) +/** + * @def CHIP_ERROR_SHUT_DOWN + * + * @brief + * The operation cancelled because a shut down was initiated + */ +#define CHIP_ERROR_SHUT_DOWN CHIP_CORE_ERROR(0x73) + +/** + * @def CHIP_ERROR_SHUT_DOWN + * + * @brief + * The operation has been cancelled, generally by calling a cancel/abort request. + */ +#define CHIP_ERROR_CANCELLED CHIP_CORE_ERROR(0x74) /** * @def CHIP_ERROR_DRBG_ENTROPY_SOURCE_FAILED @@ -2408,7 +2421,6 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_BUSY CHIP_CORE_ERROR(0xdb) - /** * @} */