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

Flush pending deregistrations prior to stopping #17

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

Conversation

twasilczyk
Copy link

Original change on AOSP

@Abhayakara
Copy link
Owner

Abhayakara commented Oct 13, 2021

I'm not doubting that it is, but can you talk a bit about why this is necessary? It will help when I try to explain the proposed change to the group.

(That is, what was the symptom you were seeing that led to deciding to make this change)

@twasilczyk
Copy link
Author

This was an old bug (fixed at our side almost 7 years ago) where the original author doesn't precisely remember it.

The initial problem was that our Nearby Connections API didn't get onEndpointLost call triggered. It seems that each advertisement has a TTL of 2min and the goodbye message is a way to let scanners know early, instead of waiting for that 2 minute expiration. This greatly improves the responsiveness of the scanning device.

@olivierlevon
Copy link

Note : Old mdnsresponder code (https://opensource.apple.com/source/mDNSResponder/mDNSResponder-107.6/mDNSCore/mDNS.c.auto.html) had this SendResponses line. Look at mDNS_Close function :

if (m->ResourceRecords) debugf("mDNS_Close: Sending final packets for deregistering records");
else debugf("mDNS_Close: No deregistering records remain");

// If any deregistering records remain, send their deregistration announcements before we exit
if (m->mDNSPlatformStatus != mStatus_NoError) DiscardDeregistrations(m);
else if (m->ResourceRecords) SendResponses(m);
if (m->ResourceRecords) LogMsg("mDNS_Close failed to send goodbye for: %s", ARDisplayString(m, m->ResourceRecords));)

@twasilczyk
Copy link
Author

Do we need anything else to merge this?

@Abhayakara
Copy link
Owner

No, I just need to get done with my current development push. Sorry for the delay!

@twasilczyk
Copy link
Author

Hey, it's been a while. While I'll be putting this work on Android side on hold for now (it's been effectively on hold anyway since 2021), just checking in if anything changed about the 6 open PRs.

@Abhayakara
Copy link
Owner

We’re still trying to get a better answer to the open source management question, but haven’t had time yet. Thread has been consuming all available bandwidth. :(

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

Successfully merging this pull request may close these issues.

3 participants