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

[mdns] unify the behavior of Stop() between avahi and mDNSResponder #2725

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

superwhd
Copy link
Contributor

@superwhd superwhd commented Feb 18, 2025

In the Stop() method, the Publisher will:

  1. Early exit if it's already stopped.
  2. Mark the state as stopped.
  3. Clear registrations and subscriptions. The destructors will free the mDNS library's client-side objects.
  4. Trigger state callbacks.

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.60%. Comparing base (2b41187) to head (af1a692).
Report is 954 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2725       +/-   ##
===========================================
- Coverage   55.77%   43.60%   -12.17%     
===========================================
  Files          87      108       +21     
  Lines        6890    13355     +6465     
  Branches        0      953      +953     
===========================================
+ Hits         3843     5824     +1981     
- Misses       3047     7223     +4176     
- Partials        0      308      +308     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@superwhd superwhd marked this pull request as ready for review February 20, 2025 02:23
@superwhd superwhd requested review from abtink and removed request for abtink February 20, 2025 02:24
@superwhd superwhd requested a review from abtink February 20, 2025 07:56
@@ -241,6 +241,8 @@ void PublisherMDnsSd::Stop(StopMode aStopMode)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A general question about how the Stop() method is intended to be used:

  1. Stop() can be called when the underlying mDNS provider (e.g., mDNSResponder) is itself stopped.
  2. It may be called when the underlying mDNS provider is still active and running, but for some other reason, we are stopping all mDNS operations.

I noticed we have StopMode input with Stop(kNormalStop) or kStopOnServiceNotRunningError, which makes it seem that both cases are used and should be supported.

If we need to support both cases (which I think we need to), then I think we should handle them differently.

  • In the first case, it is okay/desired to set mState early to ensure the Clear() methods (destructors and Unregister() calls) won't call mDNSResponder APIs anymore, as it is no longer running.
  • In the second case, we do want to call mDNSResponder APIs to clear any previously registered entries (otherwise, it continues to publish the service/host info). But then setting mState early in the method will cause these to be skipped (which would be incorrect).

My suggestion is that we track whether the mDNSResponder is active in a separate variable other than mState, which we then check (along with mState) to decide if we can call any of the mDNSResponder (or Avahi) APIs. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants