Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the Darwin logging when we cancel dns-sd browse/resolve operations. #26072

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions src/platform/Darwin/DnssdContexts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,25 +112,35 @@ DNSServiceProtocol GetProtocol(const chip::Inet::IPAddressType & addressType)
namespace chip {
namespace Dnssd {

CHIP_ERROR GenericContext::Finalize(DNSServiceErrorType err)
CHIP_ERROR GenericContext::FinalizeInternal(const char * errorStr, CHIP_ERROR err)
{
if (MdnsContexts::GetInstance().Has(this) == CHIP_NO_ERROR)
{
if (kDNSServiceErr_NoError == err)
if (CHIP_NO_ERROR == err)
{
DispatchSuccess();
}
else
{
DispatchFailure(err);
DispatchFailure(errorStr, err);
}
}
else
{
chip::Platform::Delete(this);
}

return Error::ToChipError(err);
return err;
}

CHIP_ERROR GenericContext::Finalize(CHIP_ERROR err)
{
return FinalizeInternal(err.AsString(), err);
}

CHIP_ERROR GenericContext::Finalize(DNSServiceErrorType err)
{
return FinalizeInternal(Error::ToString(err), Error::ToChipError(err));
}

MdnsContexts::~MdnsContexts()
Expand Down Expand Up @@ -289,10 +299,10 @@ RegisterContext::RegisterContext(const char * sType, const char * instanceName,
mInstanceName = instanceName;
}

void RegisterContext::DispatchFailure(DNSServiceErrorType err)
void RegisterContext::DispatchFailure(const char * errorStr, CHIP_ERROR err)
{
ChipLogError(Discovery, "Mdns: Register failure (%s)", Error::ToString(err));
callback(context, nullptr, nullptr, Error::ToChipError(err));
ChipLogError(Discovery, "Mdns: Register failure (%s)", errorStr);
callback(context, nullptr, nullptr, err);
MdnsContexts::GetInstance().Remove(this);
}

Expand All @@ -316,10 +326,10 @@ BrowseContext::BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServ
protocol = cbContextProtocol;
}

void BrowseContext::DispatchFailure(DNSServiceErrorType err)
void BrowseContext::DispatchFailure(const char * errorStr, CHIP_ERROR err)
{
ChipLogError(Discovery, "Mdns: Browse failure (%s)", Error::ToString(err));
callback(context, nullptr, 0, true, Error::ToChipError(err));
ChipLogError(Discovery, "Mdns: Browse failure (%s)", errorStr);
callback(context, nullptr, 0, true, err);
MdnsContexts::GetInstance().Remove(this);
}

Expand Down Expand Up @@ -353,14 +363,14 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::

ResolveContext::~ResolveContext() {}

void ResolveContext::DispatchFailure(DNSServiceErrorType err)
void ResolveContext::DispatchFailure(const char * errorStr, CHIP_ERROR err)
{
ChipLogError(Discovery, "Mdns: Resolve failure (%s)", Error::ToString(err));
ChipLogError(Discovery, "Mdns: Resolve failure (%s)", errorStr);
// Remove before dispatching, so calls back into
// ChipDnssdResolveNoLongerNeeded don't find us and try to also remove us.
bool needDelete = MdnsContexts::GetInstance().RemoveWithoutDeleting(this);

callback(context, nullptr, Span<Inet::IPAddress>(), Error::ToChipError(err));
callback(context, nullptr, Span<Inet::IPAddress>(), err);

if (needDelete)
{
Expand Down
14 changes: 7 additions & 7 deletions src/platform/Darwin/DnssdImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -465,11 +465,11 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier)
return CHIP_ERROR_NOT_FOUND;
}

// Just treat this as a timeout error. Don't bother delivering the partial
// We have been canceled. Don't bother delivering the partial
// results we have queued up in the BrowseContext, if any. In practice
// there shouldn't be anything there long-term anyway.
//
// Make sure to time out all the resolves first, before we time out the
// Make sure to cancel all the resolves first, before we cancel the
// browse (just to avoid dangling pointers in the resolves, even though we
// only use them for equality compares).
std::vector<GenericContext *> resolves;
Expand All @@ -481,10 +481,10 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier)

for (auto & resolve : resolves)
{
resolve->Finalize(kDNSServiceErr_Timeout);
resolve->Finalize(CHIP_ERROR_CANCELLED);
}

ctx->Finalize(kDNSServiceErr_Timeout);
ctx->Finalize(CHIP_ERROR_CANCELLED);
return CHIP_NO_ERROR;
}

Expand All @@ -511,11 +511,11 @@ void ChipDnssdResolveNoLongerNeeded(const char * instanceName)
if (*existingCtx->consumerCounter == 0)
{
// No more consumers; clear out all of these resolves so they don't
// stick around. Dispatch timeout failure on all of them to make sure
// whatever kicked them off cleans up resources as needed.
// stick around. Dispatch a "cancelled" failure on all of them to make
// sure whatever kicked them off cleans up resources as needed.
do
{
existingCtx->Finalize(kDNSServiceErr_Timeout);
existingCtx->Finalize(CHIP_ERROR_CANCELLED);
existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(instanceName);
} while (existingCtx != nullptr);
}
Expand Down
15 changes: 10 additions & 5 deletions src/platform/Darwin/DnssdImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,14 @@ struct GenericContext

virtual ~GenericContext() {}

CHIP_ERROR Finalize(CHIP_ERROR err);
CHIP_ERROR Finalize(DNSServiceErrorType err = kDNSServiceErr_NoError);
virtual void DispatchFailure(DNSServiceErrorType err) = 0;
virtual void DispatchSuccess() = 0;

virtual void DispatchFailure(const char * errorStr, CHIP_ERROR err) = 0;
virtual void DispatchSuccess() = 0;

private:
CHIP_ERROR FinalizeInternal(const char * errorStr, CHIP_ERROR err);
};

struct RegisterContext;
Expand Down Expand Up @@ -130,7 +135,7 @@ struct RegisterContext : public GenericContext
RegisterContext(const char * sType, const char * instanceName, DnssdPublishCallback cb, void * cbContext);
virtual ~RegisterContext() { mHostNameRegistrar.Unregister(); }

void DispatchFailure(DNSServiceErrorType err) override;
void DispatchFailure(const char * errorStr, CHIP_ERROR err) override;
void DispatchSuccess() override;

bool matches(const char * sType) { return mType.compare(sType) == 0; }
Expand All @@ -145,7 +150,7 @@ struct BrowseContext : public GenericContext
BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServiceProtocol cbContextProtocol);
virtual ~BrowseContext() {}

void DispatchFailure(DNSServiceErrorType err) override;
void DispatchFailure(const char * errorStr, CHIP_ERROR err) override;
void DispatchSuccess() override;

// Dispatch what we have found so far, but don't stop browsing.
Expand Down Expand Up @@ -195,7 +200,7 @@ struct ResolveContext : public GenericContext
std::shared_ptr<uint32_t> && consumerCounterToUse);
virtual ~ResolveContext();

void DispatchFailure(DNSServiceErrorType err) override;
void DispatchFailure(const char * errorStr, CHIP_ERROR err) override;
void DispatchSuccess() override;

CHIP_ERROR OnNewAddress(uint32_t interfaceId, const struct sockaddr * address);
Expand Down