-
Notifications
You must be signed in to change notification settings - Fork 188
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 the ability to handle newly subscribed peers #190
Add the ability to handle newly subscribed peers #190
Conversation
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.
This looks good. Shouldn't we also notify when peers leave the topic however?
An issue here is that if we subscribe after peers have been discovered, there will be no notifications. |
While you could already get the existing peers with the |
@vyzo That's fine by me. Any thoughts on how important receiving all the leave messages is? In particular, while joins might be slow enough that concurrent joins will not get dropped due to channel buffer overflow disconnecting from the internet might instead lead to mass connectivity dropping and therefore some missed Also, wanted to confirm that blacklisting based disconnects should count towards |
…re we subscribe. Added a Subscription Leave event
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.
There is a race to read join and leave notifications, as they are emitted in different channels.
I think we need a single channel for both notifications and use a struct with a field that specifies the notification type.
Combined Join and Leave events into a single API with a struct that specifies whether the event is a Join or a Leave.
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.
this is looking good, just a few minor points.
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.
only thing remaining to fix is the enum, other than that it looks good.
Latest idea for how to do this: We effectively want an infinite length buffer so that no Joins/Leaves are lost. However, we can be slightly efficient by ignoring when the same peer Joins and Leaves before we even ask for a notification. Strategy: There are some side points around how to get this right and look clean that I could definitely use some review on. |
Maybe? I'm concerned about races reading the channel. Someone needs to drain the channel and we can't have both the subscriber and pubsub do this (or risk out of order events). If we want to go with this approach, we could just put everything into the map up front (and use a channel for signaling). |
subscription.go
Outdated
} | ||
|
||
select { | ||
case evt, ok := <-sub.peerEvtCh: |
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.
This is racy. We could be in the middle of draining this queue into the map and read an leave event before the join (now in the map).
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.
(sorry, we missed a case)
…acklog into eventLog. removed test that was no longer useful.
case sub.evtLogCh <- struct{}{}: | ||
default: | ||
} | ||
} |
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.
@Stebalien we thinking this works? I should only need to push a signal if there are events in the log right?
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.
LGTM.
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.
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.
alright, let's merge this.
Per conversations with @vyzo @raulk and @Stebalien we are refactoring the content of #171 which does 2 things.
Instead of doing this by making routers extensible, we will be leaving router extensibility for a later date and solve the issues above by:
go-libp2p-pubsub
package.This PR deals with the latter.