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

Make sure we clean up resolves started by a browse if they have not completed when the browse completes. #24074

Closed
bzbarsky-apple opened this issue Dec 14, 2022 · 2 comments · Fixed by #24733
Closed

Make sure we clean up resolves started by a browse if they have not completed when the browse completes. #24074

bzbarsky-apple opened this issue Dec 14, 2022 · 2 comments · Fixed by #24733
Assignees
@bzbarsky-apple
Labels
darwin

Comments

@bzbarsky-apple
Copy link
Contributor

Right now Darwin browse relies on the platform dns middle layer to do a resolve on the browse results. That means we get DNSServiceResolve calls that no one ever cancels.

We should just do the resolves internally using the same service ref as the browse, so canceling the browse cancels the resolves as well. Need to make sure all the various refcounting is correct in the caller and that we don't drop the service ref too early inside the Darwin code.
@bzbarsky-apple
Copy link
Contributor Author

@vivien-apple

@bzbarsky-apple bzbarsky-apple self-assigned this Jan 29, 2023
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jan 30, 2023
@bzbarsky-apple
Make sure we stop resolves triggered by a browse when the browse stop…
9a71775 
…s 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
@bzbarsky-apple bzbarsky-apple mentioned this issue Jan 30, 2023
Merged
@bzbarsky-apple
Copy link
Contributor Author

Using the same connection as the browse does not work without more changes for the reason described at #24733 (comment). We're going to fix this by just explicitly ending the resolves the browse triggered.

@bzbarsky-apple bzbarsky-apple changed the title Darwin browse should do a resolve on the same service ref Make sure we clean up resolves started by a browse if they have not completed when the browse completes. Jan 31, 2023
yufengwangca pushed a commit that referenced this issue Jan 31, 2023
@bzbarsky-apple
Make sure we stop resolves triggered by a browse when the browse stop…
d72050c 
…s on Darwin. (#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 #24074

* Also avoid leaking ResolveContext instances.

* Fix handling of multiple interfaces.

* Address review comment.
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.
kkasperczyk-no pushed a commit to kkasperczyk-no/sdk-connectedhomeip that referenced this issue Mar 15, 2023
@bzbarsky-apple @kkasperczyk-no
Make sure we stop resolves triggered by a browse when the browse stop…
64fa3d9 
…s on Darwin. (#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/connectedhomeip#24074

* Also avoid leaking ResolveContext instances.

* Fix handling of multiple interfaces.

* Address review comment.
kkasperczyk-no pushed a commit to kkasperczyk-no/sdk-connectedhomeip that referenced this issue Mar 15, 2023
@bzbarsky-apple @kkasperczyk-no
Make sure we stop resolves triggered by a browse when the browse stop…
f65496a 
…s on Darwin. (#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/connectedhomeip#24074

* Also avoid leaking ResolveContext instances.

* Fix handling of multiple interfaces.

* Address review comment.
lecndav pushed a commit to lecndav/connectedhomeip that referenced this issue Mar 22, 2023
@bzbarsky-apple
Make sure we stop resolves triggered by a browse when the browse stop…
8674792 
…s 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
darwin
Projects
None yet
Milestone
No milestone
1 participant
@bzbarsky-apple