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

Add client option to detect disconnected clients #31

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

betamos
Copy link

@betamos betamos commented Jul 31, 2023

Detecting disconnected clients quickly can improve UX and the perception of "snappyness", and also avoid unnecessary futile connection attempts to stale clients.

Needed this for my own purposes and fiddled around until I got a minimal non-breaking change that does the job reliably, without duplicates.

See RFC 6762 Sec 10.1: Goodbye Packet

I have verified that the server in this module correctly emits goodbye packets. This change is only for the client to detect it.

Apologies for not raising an issue beforehand.

- Added a couple of comments
- Removed superfluous delete entries while iterating
- Removed superfluous length check prior to iterating
@marten-seemann
Copy link

Thanks for the PR! We're not maintaining this package as a general-purpose zeroconf library though, we just forked it from https://github.com/grandcat/zeroconf, since we weren't able to upstream some critical changes we needed in libp2p.

Have you considered submitting your fix there?

@betamos
Copy link
Author

betamos commented Aug 1, 2023

Thanks Marten. Turns out that someone had worked on a PR for goodbye packets, and then there's your PR for TTL expiry that still isn't merged. We'll see if my bump does anything.

We're not maintaining this package as a general-purpose zeroconf library though

I understand. Have y'all had experience taking active custody of projects like this before? In spirit libp2p seems like a good home for mDNS - this fork is already the best general-purpose Go lib that I've found.

At the very least, don't hesitate to reach out for help with maintenance. I've contributed to both JS- and Go libp2p libraries before.

@marten-seemann
Copy link

It is indeed a maintenance issue. Sometimes we need to fork a library, because we need to fix a bug or add a feature, and sometimes our changes are not accepted upstream. We then end up in the unfortunate place where people start depending on our fork outside of the libp2p context. I’m not really sure what to do in this situation. We have limited resources, so we can’t really devote a lot of time for maintaining / reviewing code that works for us (zeroconf is a good example for this, we know that some issues exist here that occur in Kubo, but haven’t even had the time to debug those).

@betamos
Copy link
Author

betamos commented Aug 31, 2023

Understood.

In case it's any use to you, I should let you know I nerd-sniped myself and rewrote the whole lib: https://github.com/betamos/zeroconf/

Many bug- and behavioral fixes. Planning on supporting it for the current feature set I'm going to use it on the critical path for my app (Payload).

Do with it what you will. And feel free to close this one. Thanks!

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