Skip to content

Commit

Permalink
Add a way for Resolver consumers to cancel operational resolve attempts.
Browse files Browse the repository at this point in the history
Adds a way for consumers to notify Resolver when they no longer care
about an operational resolve, so a Resolver implementation can keep
track of how many consumers are interested and stop work as desired if
no one is interested.

Fixes project-chip#23881
  • Loading branch information
bzbarsky-apple committed Dec 10, 2022
1 parent 1c691ec commit 64c8922
Show file tree
Hide file tree
Showing 26 changed files with 272 additions and 24 deletions.
2 changes: 2 additions & 0 deletions src/controller/python/chip/discovery/NodeResolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class PythonResolverDelegate : public OperationalResolveDelegate
public:
void OnOperationalNodeResolved(const ResolvedNodeData & nodeData) override
{
Resolver::Instance().NodeIdResolutionNoLongerNeeded(nodeData.operationalData.peerId);
if (mSuccessCallback != nullptr)
{
char ipAddressBuffer[128];
Expand All @@ -59,6 +60,7 @@ class PythonResolverDelegate : public OperationalResolveDelegate

void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override
{
Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);
if (mFailureCallback != nullptr)
{
mFailureCallback(peerId.GetCompressedFabricId(), peerId.GetNodeId(), ToPyChipError(error));
Expand Down
1 change: 1 addition & 0 deletions src/controller/tests/TestCommissionableNodeController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class MockResolver : public Resolver
void SetOperationalDelegate(OperationalResolveDelegate * delegate) override {}
void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override {}
CHIP_ERROR ResolveNodeId(const PeerId & peerId) override { return ResolveNodeIdStatus; }
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override {}
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return DiscoverCommissionersStatus; }
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override
{
Expand Down
7 changes: 7 additions & 0 deletions src/lib/address_resolve/AddressResolve_DefaultImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ CHIP_ERROR Resolver::CancelLookup(Impl::NodeLookupHandle & handle, FailureCallba
{
VerifyOrReturnError(handle.IsActive(), CHIP_ERROR_INVALID_ARGUMENT);
mActiveLookups.Remove(&handle);
Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(handle.GetRequest().GetPeerId());

// Adjust any timing updates.
ReArmTimer();
Expand Down Expand Up @@ -164,6 +165,7 @@ void Resolver::Shutdown()

mActiveLookups.Erase(current);

Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);
// 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)
Expand Down Expand Up @@ -231,6 +233,8 @@ void Resolver::HandleAction(IntrusiveList<NodeLookupHandle>::Iterator & current)
NodeListener * listener = current->GetListener();
mActiveLookups.Erase(current);

Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);

// ensure action is taken AFTER the current current lookup is marked complete
// This allows failure handlers to deallocate structures that may
// contain the active lookup data as a member (intrusive lists members)
Expand Down Expand Up @@ -277,6 +281,8 @@ void Resolver::OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERR
NodeListener * listener = current->GetListener();
mActiveLookups.Erase(current);

Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);

// 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)
Expand Down Expand Up @@ -326,6 +332,7 @@ void Resolver::ReArmTimer()
mActiveLookups.Erase(it);
it = mActiveLookups.begin();

Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);
// Callback only called after active lookup is cleared
// This allows failure handlers to deallocate structures that may
// contain the active lookup data as a member (intrusive lists members)
Expand Down
17 changes: 17 additions & 0 deletions src/lib/dnssd/ActiveResolveAttempts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,22 @@ CHIP_ERROR ActiveResolveAttempts::CompleteAllBrowses()
return CHIP_NO_ERROR;
}

void ActiveResolveAttempts::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
for (auto & item : mRetryQueue)
{
if (item.attempt.Matches(peerId))
{
item.attempt.ConsumerRemoved();
if (item.attempt.ResolveData().consumerCount == 0)
{
item.attempt.Clear();
}
return;
}
}
}

void ActiveResolveAttempts::MarkPending(const chip::PeerId & peerId)
{
MarkPending(ScheduledAttempt(peerId, /* firstSend */ true));
Expand Down Expand Up @@ -172,6 +188,7 @@ void ActiveResolveAttempts::MarkPending(ScheduledAttempt && attempt)
ChipLogError(Discovery, "Re-using pending resolve entry before reply was received.");
}

attempt.WillCoalesceWith(entryToUse->attempt);
entryToUse->attempt = attempt;
entryToUse->queryDueTime = mClock->GetMonotonicTimestamp();
entryToUse->nextRetryDelay = System::Clock::Seconds16(1);
Expand Down
33 changes: 32 additions & 1 deletion src/lib/dnssd/ActiveResolveAttempts.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class ActiveResolveAttempts
struct Resolve
{
chip::PeerId peerId;
uint32_t consumerCount = 0;

Resolve(chip::PeerId id) : peerId(id) {}
};
Expand All @@ -68,7 +69,11 @@ class ActiveResolveAttempts
IpResolve(HeapQName && host) : hostName(std::move(host)) {}
};

ScheduledAttempt() {}
ScheduledAttempt()
{
static_assert(sizeof(Resolve) <= sizeof(Browse) || sizeof(Resolve) <= sizeof(HeapQName),
"Figure out where to put the Resolve counter.");
}
ScheduledAttempt(const chip::PeerId & peer, bool first) :
resolveData(chip::InPlaceTemplateType<Resolve>(), peer), firstSend(first)
{}
Expand Down Expand Up @@ -181,6 +186,28 @@ class ActiveResolveAttempts
bool IsIpResolve() const { return resolveData.Is<IpResolve>(); }
void Clear() { resolveData = DataType(); }

void WillCoalesceWith(const ScheduledAttempt & existing)
{
if (IsResolve() && existing.Matches(*this))
{
resolveData.Get<Resolve>().consumerCount = existing.resolveData.Get<Resolve>().consumerCount + 1;
}
}

void ConsumerRemoved()
{
if (!IsResolve())
{
return;
}

auto & count = resolveData.Get<Resolve>().consumerCount;
if (count > 0)
{
--count;
}
}

const Browse & BrowseData() const { return resolveData.Get<Browse>(); }
const Resolve & ResolveData() const { return resolveData.Get<Resolve>(); }
const IpResolve & IpResolveData() const { return resolveData.Get<IpResolve>(); }
Expand Down Expand Up @@ -208,6 +235,10 @@ class ActiveResolveAttempts
/// from the internal list.
CHIP_ERROR CompleteAllBrowses();

/// Note that resolve attempts for the given peer id now have one fewer
/// consumer.
void NodeIdResolutionNoLongerNeeded(const chip::PeerId & peerId);

/// Mark that a resolution is pending, adding it to the internal list
///
/// Once this complete, this peer id will be returned immediately
Expand Down
30 changes: 27 additions & 3 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,15 @@ CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId)
return mResolverProxy.ResolveNodeId(peerId);
}

void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
char name[Common::kInstanceNameMaxLength + 1];
if (MakeInstanceName(name, sizeof(name), peerId) == CHIP_NO_ERROR)
{
ChipDnssdResolveNoLongerNeeded(name);
}
}

CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissionableNodes(DiscoveryFilter filter)
{
ReturnErrorOnFailure(InitImpl());
Expand Down Expand Up @@ -671,15 +680,30 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId)
ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...",
ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()));

mDelegate->Retain();

DnssdService service;

ReturnErrorOnFailure(MakeInstanceName(service.mName, sizeof(service.mName), peerId));
Platform::CopyString(service.mType, kOperationalServiceName);
service.mProtocol = DnssdServiceProtocol::kDnssdProtocolTcp;
service.mAddressType = Inet::IPAddressType::kAny;
return ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, mDelegate);

mDelegate->Retain();

CHIP_ERROR err = ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, mDelegate);
if (err != CHIP_NO_ERROR)
{
mDelegate->Release();
}
return err;
}

void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
char name[Common::kInstanceNameMaxLength + 1];
if (MakeInstanceName(name, sizeof(name), peerId) == CHIP_NO_ERROR)
{
ChipDnssdResolveNoLongerNeeded(name);
}
}

ResolverProxy::~ResolverProxy()
Expand Down
1 change: 1 addition & 0 deletions src/lib/dnssd/Discovery_ImplPlatform.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
mResolverProxy.SetCommissioningDelegate(delegate);
}
CHIP_ERROR ResolveNodeId(const PeerId & peerId) override;
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override;
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR StopDiscovery() override;
Expand Down
24 changes: 22 additions & 2 deletions src/lib/dnssd/Resolver.h
Original file line number Diff line number Diff line change
Expand Up @@ -369,11 +369,31 @@ class Resolver
* This will trigger a DNSSD query.
*
* When the operation succeeds or fails, and a resolver delegate has been registered,
* the result of the operation is passed to the delegate's `OnNodeIdResolved` or
* `OnNodeIdResolutionFailed` method, respectively.
* the result of the operation is passed to the delegate's `OnOperationalNodeResolved` or
* `OnOperationalNodeResolutionFailed` method, respectively.
*
* Multiple calls to ResolveNodeId may be coalesced by the implementation
* and lead to just one call to
* OnOperationalNodeResolved/OnOperationalNodeResolutionFailed, as long as
* the later calls cause the underlying querying mechanism to re-query as if
* there were no coalescing.
*
* A single call to ResolveNodeId may lead to multiple calls to
* OnOperationalNodeResolved with different IP addresses.
*
* @see NodeIdResolutionNoLongerNeeded.
*/
virtual CHIP_ERROR ResolveNodeId(const PeerId & peerId) = 0;

/*
* Notify the resolver that one of the consumers that called ResolveNodeId
* successfully no longer needs the resolution result (e.g. because it got
* the result, or got an error, or no longer cares about future updates).
* There must be a NodeIdResolutionNoLongerNeeded call that matches every
* successful ResolveNodeId call.
*/
virtual void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) = 0;

/**
* Finds all commissionable nodes matching the given filter.
*
Expand Down
1 change: 1 addition & 0 deletions src/lib/dnssd/ResolverProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ class ResolverProxy : public Resolver
}

CHIP_ERROR ResolveNodeId(const PeerId & peerId) override;
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override;
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR StopDiscovery() override;
Expand Down
11 changes: 11 additions & 0 deletions src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate
void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mOperationalDelegate = delegate; }
void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override { mCommissioningDelegate = delegate; }
CHIP_ERROR ResolveNodeId(const PeerId & peerId) override;
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override;
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR StopDiscovery() override;
Expand Down Expand Up @@ -659,6 +660,11 @@ CHIP_ERROR MinMdnsResolver::ResolveNodeId(const PeerId & peerId)
return SendAllPendingQueries();
}

void MinMdnsResolver::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
mActiveResolves.NodeIdResolutionNoLongerNeeded(peerId);
}

CHIP_ERROR MinMdnsResolver::ScheduleRetries()
{
ReturnErrorCodeIf(mSystemLayer == nullptr, CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -710,6 +716,11 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId)
return chip::Dnssd::Resolver::Instance().ResolveNodeId(peerId);
}

void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
return chip::Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);
}

CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
{
VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
Expand Down
6 changes: 6 additions & 0 deletions src/lib/dnssd/Resolver_ImplNone.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class NoneResolver : public Resolver
ChipLogError(Discovery, "Failed to resolve node ID: dnssd resolving not available");
return CHIP_ERROR_NOT_IMPLEMENTED;
}
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override
{
ChipLogError(Discovery, "Failed to stop resolving node ID: dnssd resolving not available");
}
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override
{
return CHIP_ERROR_NOT_IMPLEMENTED;
Expand Down Expand Up @@ -68,6 +72,8 @@ CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId)
return CHIP_ERROR_NOT_IMPLEMENTED;
}

void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) {}

CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
Expand Down
7 changes: 7 additions & 0 deletions src/lib/dnssd/platform/Dnssd.h
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,13 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier);
CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId interface, DnssdResolveCallback callback,
void * context);

/**
* This function notifies the implementation that a resolve result is no longer
* needed by some consumer, to allow implementations to stop unnecessary resolve
* work.
*/
void ChipDnssdResolveNoLongerNeeded(const char * instanceName);

/**
* This function asks the mdns daemon to asynchronously reconfirm an address that appears to be out of date.
*
Expand Down
8 changes: 6 additions & 2 deletions src/lib/shell/commands/Dns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@ namespace {

Shell::Engine sShellDnsBrowseSubcommands;
Shell::Engine sShellDnsSubcommands;
Dnssd::ResolverProxy sResolverProxy;

class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, public Dnssd::CommissioningResolveDelegate
{
public:
void OnOperationalNodeResolved(const Dnssd::ResolvedNodeData & nodeData) override
{
sResolverProxy.NodeIdResolutionNoLongerNeeded(nodeData.operationalData.peerId);
streamer_printf(streamer_get(), "DNS resolve for " ChipLogFormatX64 "-" ChipLogFormatX64 " succeeded:\r\n",
ChipLogValueX64(nodeData.operationalData.peerId.GetCompressedFabricId()),
ChipLogValueX64(nodeData.operationalData.peerId.GetNodeId()));
Expand All @@ -66,7 +68,10 @@ class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, publi
streamer_printf(streamer_get(), " Supports TCP: %s\r\n", nodeData.resolutionData.supportsTcp ? "yes" : "no");
}

void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override {}
void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override
{
sResolverProxy.NodeIdResolutionNoLongerNeeded(peerId);
}

void OnNodeDiscovered(const Dnssd::DiscoveredNodeData & nodeData) override
{
Expand Down Expand Up @@ -116,7 +121,6 @@ class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, publi
};

DnsShellResolverDelegate sDnsShellResolverDelegate;
Dnssd::ResolverProxy sResolverProxy;

CHIP_ERROR ResolveHandler(int argc, char ** argv)
{
Expand Down
2 changes: 2 additions & 0 deletions src/platform/Ameba/DnssdImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,5 +114,7 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * /*service*/, chip::Inet::InterfaceId
return CHIP_ERROR_NOT_IMPLEMENTED;
}

void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {}

} // namespace Dnssd
} // namespace chip
Loading

0 comments on commit 64c8922

Please sign in to comment.