Skip to content

Commit

Permalink
Improve the Darwin logging when we cancel dns-sd browse/resolve opera…
Browse files Browse the repository at this point in the history
…tions.

Right now we are treating those as "timeout" and logging that, which really
confuses people reading the logs.  We should log them as cancellations instead.

There's no obvious kDNSServiceErr_* value for "cancelled", so allow doing
GenericContext::Finalize with a CHIP_ERROR.
  • Loading branch information
bzbarsky-apple committed Apr 12, 2023
1 parent 2ce04b5 commit 86a6d62
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 25 deletions.
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

0 comments on commit 86a6d62

Please sign in to comment.