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

handle local name collision and avahi-daemon restart #1294

Closed
wants to merge 1 commit into from

Conversation

wellenvogel
Copy link

#1293 - retry on registration and re-register on errors

@wellenvogel
Copy link
Author

Maybe it would additionally make sense to be able to change the service names that SignalK is using.
Assuming you would have 2 services on the same system providing Web-Access (_http._tcp) - like e.g. on OpenPlotter - it would make a lot of sense that you could distinguish them by their service names. Maybe the default for signalk could even be "signalk" instead of using the hostname (you will still use the hostname as the address). Currently you could change the external hostname - but this seems to be confusing - in the above scenario the two services would use different hostnames.

@tkurki
Copy link
Member

tkurki commented Mar 15, 2021

Note that I am trying to use terminology from https://tools.ietf.org/html/rfc6763#section-4.1 and mdns API uses different terms in its API.

Service Instance Name = <Instance> . <Service> . <Domain>
Example:
Stuart's\032Printer._http._tcp.dns-sd.org

Is it so that (one?) root cause of the problem is that we advertising with Service _http._tcp with no Instance? And what your patch does is that if there is conflict (somebody else is advertising the same Service, with an empty / no Instance and same host?) it retries with Instance name made from hostname?

What if we would advertise from the start like the example from the rfc, with a human oriented instance name? Use Signal K Server initially and with a length-limited, sanitised vessel name if there is one.

This would probably eliminate the problem almost totally, but the retry is also good to add.

@wellenvogel
Copy link
Author

Basically at the end you have a couple of entities:

  • You have the service type like _http._tcp. (service name 4.1.2)
  • You have a name for the service (that tools often display). In your implementation it defaults to the hostname (and basically could not be changed at all) - instance name 4.1.1
  • You have a hostname - also defaults to the hostname
  • You have a domain (defaults to local) - 4.1.3
  • You have a port
    I guess there is no problem with the service type (not sure how the intention is for clients - like e,g, SenseESP - are they just querying by service type?). You anyway have a couple of SK specific types...
    And for types like _http._tcp - they are somehow standardized. And there are apps out there relying on such types.
    For the service name I'm a bit unsure what the best approach is. On one hand side if users only see the service name in some tools (4.2.) it would be great to get an idea about the service and the host. On the other hand if multiple services are running on the same host and they all e.g. provide some service of type _http._tcp - would be great to distinguish them. So maybe even a combination of a service description and a hostname would be nice (like signalk-web-myhost - or at least signalk-myhost). But again: How would clients query?
    The retry is described in appendix D of the rfc.
    If we would be strict on that we even would have to store the selected name somehow in a persistent manner.
    But maybe the idea of using the vessel name is also great (most probably not 2 SK servers on one vessel ;-)).

For the hostname: Finally I would just leave this to mdns/avahi to pick a nice hostname. This way at least all services on the same host would utilize the same hostname.

@tkurki
Copy link
Member

tkurki commented Apr 6, 2021

Could you please check the changes in #1300? Now that it does not use hostname as instancename are conflicts still happening? I don't have a Pi handy right now to test avahi daemon restart.

I suppose the fallback to appending numbers in case of conflicts should also be added.

@wellenvogel
Copy link
Author

I guess the changes in #1300 still leave a couple of problems open:

  1. if the name already exists (for whatever reason) ad.start will throw an exception. This will prevent other services from being registered.
  2. the spec describes that you should retry registering with a changed name if the name already exists
  3. it still does not handle the avahi-daemon restart - this will fire the onError of the advertisement.

@tkurki
Copy link
Member

tkurki commented Apr 7, 2021

Yes.

@tkurki
Copy link
Member

tkurki commented Apr 7, 2021

Replaced by #1300 .

@tkurki tkurki closed this Apr 7, 2021
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.

2 participants