Subscribe to a topic multiple times #10
Conversation
|
paging @whyrusleeping |
|
CR please :) |
| for t := range p.myTopics { | ||
| out = append(out, t) | ||
| for t, subs := range p.myTopics { | ||
| if len(subs) > 0 { |
There was a problem hiding this comment.
should this ever not be > 0? That seems like it could end up being a memory leak
There was a problem hiding this comment.
I just searched for occurences of myTopics and it seems there is no way that it's empty. I'll just drop the check. Thanks!
| func (p *PubSub) GetTopics() []string { | ||
| out := make(chan []string, 1) | ||
| p.getTopics <- &topicReq{resp: out} | ||
| return <-out | ||
| } | ||
|
|
||
| func (p *PubSub) SubscribeComplicated(td *pb.TopicDescriptor) (<-chan *Message, error) { |
There was a problem hiding this comment.
We still need the topic descriptors. They arent super used yet, but thats the next step for this impl.
|
@keks the approach looks good to me, i'd like to see some new tests exercising that behaviour from a users perspective though (as in, how a consumer would use multiple subscriptions) |
|
@whyrusleeping there you go! |
|
I added the test showcasing that you can now subscribe to one topic multiple times. I guess in the test it doesn't look very impressing, but in the ipfs daemon it's important we can do that because the might be pubsub clients that subscribe to the same topic but don't even know about each other. |
| resp chan []string | ||
| } | ||
| // Subscribe returns a new Subscription for the given topic | ||
| func (p *PubSub) Subscribe(topic string) *Subscription { |
There was a problem hiding this comment.
We should ensure that all subscribe calls go through SubscribeByTopicDescriptor, it will make things easier on us moving forward
|
@keks fantastic, thank you! One more comment and then LGTM |
|
Done. Note this introduces an API change since |
|
There you go... |
|
I rebased and added the context to |
|
I'm ready to merge this, just gotta catch it right after youve rebased. |
37c35e0 to
21adc8d
Compare
|
sigh I'll figure it out eventually... |
|
actually I don't get the problem. The builders complain they can't find the libp2p gx dependency, but that change happened in c0c5a38. The tests don't even start. Can you kick it so it tries again @whyrusleeping? |
|
@keks I think that his package might have been moved to: https://github.com/libp2p/go-libp2p-netutil |
|
And it might have been my fail on not fixing this up before updating the dep. |
|
@keks use |
|
You can also give me push access to this branch and i can fix it (alternatively, why are you not a contributor yet? let me fix that) |
|
🎉 |
64e4402 to
4e943ef
Compare
|
Tests pass, no conflicts and gx published...I guess this means ready to merge? What do you think @whyrusleeping? |
This PR basically does what I explained here. I rebased everything so that it is easily mergable. Yay!