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.

Sign up for GitHub

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

Jump to bottom

Darwin leaks DNSServiceResolve calls #23881

Closed
bzbarsky-apple opened this issue Dec 2, 2022 · 1 comment · Fixed by #24010
Closed

Darwin leaks DNSServiceResolve calls #23881

bzbarsky-apple opened this issue Dec 2, 2022 · 1 comment · Fixed by #24010
Labels
darwin leak Memory leak bug v1.1

Comments

@bzbarsky-apple
Copy link
Contributor

If a resolve in the sense of DNSServiceResolve does not complete, we never cancel it. We can keep accumulating them until things fall over.

Proposed plan for fixing this:

  1. Remove the IPAddressType argument from Dnssd::Resolver::ResolveNodeId.
  2. Add a CancelResolve (name TBD) API on Dnssd::Resolver, which takes a PeerId. This will be called from the default AddressResolve::Resolver impl when things are canceled or time out.
  3. Clearly document the expectation that Dnssd::Resolver implementations must count the number of interested consumers for an operational lookup attempt, treating ResolveNodeId as a signal to increase the consumer count and treating CancelResolve as a signal to decrease the consumer count.
  4. ResolveNodeId should be documented to kick an existing lookup to try more aggressively if it was doing backoff. Minimal mdns seems to be doing this already.
  5. ChipDnssdResolve should be documented as allowing coalescing of resolves as long as the context, and peer id are the same and as long as each call kicks things that are doing backoff as needed.
  6. Add ChipDnssdCancelResolve that takes a context and peer id.

Then we can clean things up when consumers no longer care about them.
@bzbarsky-apple
Copy link
Contributor Author

@vivien-apple

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Dec 10, 2022
@bzbarsky-apple
Add a way for Resolver consumers to cancel operational resolve attempts.
6c3713a 
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
@bzbarsky-apple bzbarsky-apple mentioned this issue Dec 10, 2022
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Dec 10, 2022
@bzbarsky-apple
Add a way for Resolver consumers to cancel operational resolve attempts.
fd8b420 
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
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Dec 10, 2022
@bzbarsky-apple
Add a way for Resolver consumers to cancel operational resolve attempts.
64c8922 
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
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Dec 10, 2022
@bzbarsky-apple
Add a way for Resolver consumers to cancel operational resolve attempts.
3d1a35c 
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
bzbarsky-apple added a commit that referenced this issue Dec 13, 2022
@bzbarsky-apple
Add a way for Resolver consumers to cancel operational resolve attemp…
9e371e6 
…ts. (#24010)

* Add a way for Resolver consumers to cancel operational resolve attempts.

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 #23881

* Address review comments.

* Address review comments.
rawadhilal88 pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Feb 22, 2023
@bzbarsky-apple
[DO-NOT-MERGE-TO-MAINLINE] This change is already in master branch th…
07a83e7 
…at fixes DNS-SD browsing

Add an API to stop a DNS-SD browse operation. (project-chip#22823)

* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.

[darwin] Use DNSServiceReconfirmRecord for A and AAAA records to miti… (project-chip#23067)

* [Dnssd] Add ReconfirmRecord method to verify address that appears to be out of date

* [SetUpCodePairer] Ask Dnssd to reconfirm discovered addresses if connecting to them ends with a CHIP_ERROR_TIMEOUT

Fix Logging When Trying to Log Nullptr To Strings (project-chip#23604)

This PR attempts to identify all cases where %s specifiers in the logging APIs
(ChipLogError(), ChipLogProgress(), ChipLogDetail()) don't have a guaranteed
non-null string parameter.

In all identified cases the issue is fixed using StringOrNullMarker() helper
method to guarantee it doesn't happen.

Use the "right" byte-swapping function for port in Darwin DnssdImpl. (project-chip#23894)

The incoming port is in host byte order and we are converting to network byte
order, so should use htons (which happens to do the same thing as ntohs, so no
behavior change).

Co-authored-by: Andrei Litvin <[email protected]>

Add a way for Resolver consumers to cancel operational resolve attempts. (project-chip#24010)

* Add a way for Resolver consumers to cancel operational resolve attempts.

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

* Address review comments.

* Address review comments.

Make sure we stop resolves triggered by a browse when the browse stops on Darwin. (project-chip#24733)

* Make sure we stop resolves triggered by a browse when the browse stops on Darwin.

Without this change, if there is a PTR record that matches whatever we are
browsing but no corresponding SRV record, we would end up leaking a resolve
forever.

Tested by modifying minimal mdns SrvResponder::AddAllResponses to no-op instead
of actually adding any responses, then trying to commission the device running
the modified minimal mdns.  Without this change, when the browse stops the
resolves it triggered keep going.  With this change, termination of the browse
also terminates the resolves.

Fixes project-chip#24074

* Also avoid leaking ResolveContext instances.

* Fix handling of multiple interfaces.

* Address review comment.

Improve discovery logging on Darwin. (project-chip#24846)

1) Use progress, not detail, logging, because detail logging is not actually
   persisted in system logs.
2) Add logging to a few functions that were missing it.

Remove the address type argument from ResolveNodeId. (project-chip#24006)

All consumers were passing kAny in practice, and some of the backends
(e.g. minimal mdns) had no capability to filter by type anyway.
rawadhilal88 pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Feb 22, 2023
@bzbarsky-apple
Add a way for Resolver consumers to cancel operational resolve attemp…
1a3b100 
…ts. (project-chip#24010)

* Add a way for Resolver consumers to cancel operational resolve attempts.

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

* Address review comments.

* Address review comments.
sharadb-amazon pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Mar 9, 2023
@bzbarsky-apple @sharadb-amazon
Add a way for Resolver consumers to cancel operational resolve attemp…
f9896e6 
…ts. (project-chip#24010)

* Add a way for Resolver consumers to cancel operational resolve attempts.

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

* Address review comments.

* Address review comments.
sharadb-amazon pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Mar 17, 2023
@bzbarsky-apple @sharadb-amazon
Add a way for Resolver consumers to cancel operational resolve attemp…
b0d2cf3 
…ts. (project-chip#24010)

* Add a way for Resolver consumers to cancel operational resolve attempts.

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

* Address review comments.

* Address review comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
darwin leak Memory leak bug v1.1
Projects
None yet
Milestone
No milestone
1 participant
@bzbarsky-apple