-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Implement MdnsDiscovery #177
Conversation
Co-authored-by: tomasciccola <tomasciccola@users.noreply.github.com>
On this last commit I'm having this issue where on an initiated connection |
Heh, had a really silly bug passing the connection handler to the tcp client. Still getting some duplicated connections though... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this - this is tricky stuff to get right. Left a bunch of comments that hopefully get this working - I can be available to work together on this today if needed.
src/discovery/mdns.js
Outdated
const socket = net.connect( | ||
service.port, | ||
service.addresses && service.addresses[0] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
) | |
socket.once('connect', () => { |
src/discovery/mdns.js
Outdated
}) | ||
|
||
secretStream.on('close', () => { | ||
this.#connections.delete(remoteAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to test what happens when:
- secretStream throws an error
- socket throws an error before secretStream connects
Regardless I believe we need to attach an error listener to secretStream - Node has a funny thing where you need to attach error listeners or it will throw an uncaught error and crash.
this.#connections.delete(remoteAddress) | |
this.#socketConnections.delete(remoteAddress) | |
this.#noiseConnections.delete(secretStream.remotePublicKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this - this is tricky stuff to get right. Left a bunch of comments that hopefully get this working - I can be available to work together on this today if needed.
I've been noticing instability in terms of peers finding each other. With no changes to the code it seems that sometimes two peers find each other, and sometimes they don't... |
…-discovery try to use those declarations and failed
Try logging the initial dedupe based on remoteaddress. I have a feeling that there is a race condition here where both clients can end up closing both sockets. It might be better to remove this check altogether and just use the dedupe on the noise stream. |
Co-authored-by: tomasciccola <tomasciccola@users.noreply.github.com>
I dunno if this is the best solution (or if `secretStream` and `server` should share the same `close` handler), but it works...
It seems that hypercore replication from on instance to another is failing. There's probably a bug I haven't catch yet...
Missing for this to be merged are more tests (testing with more peers, and probably going to look to test that exist on |
It was on the `handleConnection` which meant adding unnecessary handlers for the same event (for each peer connected)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests still need some work.
What we need to test is:
- Every device will find every other device and establish a connection
- There is no more than one connection between each device
We want to test this in a scenario that is as close as we can get to how the app will be used.
The current multi-connection test waits for 10 connection events and then stops, and doesn't do any checks about whether all clients have connected or whether there are any duplicate connections. 10 connection events = 5 connections. For 10 peers to connect to each other we need n(n-1)/2 connections, or n(n - 1) connection events.
In order to test for duplicate connections there is no great way to do this other than just "wait" for a certain period of time. I think 5 seconds should be enough.
To simulate a real-life situation, we should not start all the discoveries at the same time - although that is worth testing too, because simultaneous start can catch certain race conditions.
One way of doing all this:
- Start each discovery at a random time in the first 2 seconds of the test
- On each connection event push the
[localPublicKey, remotePublicKey]
tuple to an array. - Wait until 5 seconds total have passed.
- Check the array of connection events has
nPeers * (nPeers - 1)
entries - Check that each peer is connected to every other peer e.g.
for (publicKey of peers) {
const peerConnections = allConnections.filter(([localKey, remoteKey]) => localKey === publicKey).map(([ , remoteKey]) => remoteKey)
// Check peerConnections is the same as peers.filter(peerKey => peerKey !== publicKey)
}
If you need help with this I can work on this with you.
I don't think the hypercore replication test is necessary. We will test for that in the replication code - it's not testing anything in the MdnsDiscovery class: it's testing whether you can replicate a hypercore over a noisestream.
src/discovery/mdns.js
Outdated
log('started advertiser for ' + addr.port) | ||
|
||
// find all peers adverticing Mapeo | ||
this.#browser = new dnssd.Browser(dnssd.tcp(SERVICE_NAME)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we start browsing immediately (e.g. before waiting for the server to be listening). Also, don't wait for browser.start() to resolve before starting the server and advertiser e.g.
const browserStartPromise = this.#browser.start()
// ... start server and advertiser
await Promise.all([
browserStartPromise,
this.#advertiser.start()
])
Just to make sure that we start everything as soon as possible
src/discovery/mdns.js
Outdated
'serviceUp', | ||
/** @param {Service} service */ | ||
(service) => { | ||
if (service.txt?.id === this.#id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do do with services without a txt record? Ignore?
src/discovery/mdns.js
Outdated
} ${socket.remotePort}` | ||
) | ||
if (!socket.remoteAddress) return | ||
if (!isIpPrivate(socket.remoteAddress)) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a log here - might be useful in the future for debugging
MDNS Discovery
mapeo
. Currently module we are using: https://gitlab.com/gravitysoftware/dnssd.jsThis should close #160