Refactor to allow out-of-tree router implementations #199

vyzo · 2019-09-23T11:06:53Z

By popular demand, this refactors the pubsub system to allow out-of-tree router implementations.
This is accomplished by introducing a PubSubControl interface, which is the pubsub view presented to routers.
It is currently sufficient to implement our in-tree routers, but we may want to extend it in the future.
It also adds a new field to the protobuf, extData, to be used by out-of-tree protocols for their own purposes.

Stebalien

❤️

I believe we need to export from from pubsub.RPC.

Stebalien · 2019-09-23T17:38:23Z

pubsub.go

+
+	// Eval schedules a thunk for evaluation in the pubsub process loop.
+	// Returns false if the evaluation was aborted because the pubsub system has been shutdown.
+	Eval(thunk func()) bool


This is funky.

Are we allowed to use the rest of the interface outside of a "thunk"?

Are we using this to protect gossipsub state? If so, what if we just used a lock?

The interface in general is unsafe to use outside of the event loop, this should probably be specified in the documentation; will fix.

Got it. I wonder if we should put that part into a separate interface and pass it to the thunk? That should make it clear that that's the only safe way to use it.

Does Eval wait for the thunk to eval?

It waits for the evaluation to start, but it doesn't wait for completion.

Marked the unsafe outside the event loop methods as such.

I'd still like to consider moving these to a separate interface and passing it into the thunk. I really wish go had some kind of goroutine local storage so we could just make all this automatic but it doesn't.

Well, the interface gets attached in Router.Attach and can get used without explicitly re-entering with Eval -- all callbacks to the router from the pubsub system are inside the event loop and that would be awkward to use.
I think the contract should be "you call the pubsub system inside the event loop; if you have a goroutine that is outside (eg a timer, like in gossipsub), use Eval to re-enter the event loop."
We should probably document this in the Router interface.

Discussed synchronously: All the router callbacks are also called on the main thread so we'd have to feed this new interface in there as well. TL;DR: not worth it.

Instead, we should:

Carefully document this.

Maybe call this InvokeInMainThread (or something less java-y).

I think this is a smell. Alternative design proposal in comments above.

Stebalien · 2019-09-23T17:40:08Z

pb/rpc.proto

@@ -12,6 +12,7 @@ message RPC {
 	}

 	optional ControlMessage control = 3;
+    optional bytes extData = 4; // protocol extension data


https://developers.google.com/protocol-buffers/docs/proto3#any?

oops! emacs did this.

(use-package go-mode :config (gofmt-before-save))

😉

Any is probably a bad idea because it needs metadata, which may not be supported by some programming languages.
Also, our protobuf is proto2 so we don't want to pollute with proto3 constructs.

I do have a gofmt save hook, but this is a protobuf file!

:roll:

So, I'm just cheating and using spacemacs. (add-to-list 'protobuf-mode-hooks (lambda () (setq-local indent-tabs-mode t)))?

fixed the indentation; the hook looks good, i might adopt it :p

vyzo · 2019-09-23T17:52:41Z

Good point, the from field will have to be exported.

…event loop

Stebalien · 2019-09-23T23:17:35Z

pubsub.go

+var ErrQueueFull = errors.New("queue full")
+
+func (p *PubSubControlView) PeersForTopic(topic string) (map[peer.ID]struct{}, bool) {
+	tmap, ok := p.topics[topic]


Copying the peer list into a slice shouldn't really cost us anything in the grand scheme of things. I'd do that to avoid exposing internal state.

I'm assuming this method is on the hot path, hence the intention to avoid copying by introducing the thunk scheduling to guard state...

it is on the hot path indeed.

raulk

TL;DR: I think the Eval() is a smell. Its goal is to curtail state leakage caused the preexisting design. An event-based model would be cleaner and more comfortable to develop against.

raulk · 2019-10-01T12:07:45Z

pubsub.go

+	// PeersForTopic returns the set of peers in a topic; the boolean indicator will be false
+	// if the topic is unknown.
+	// This method is not safe to call outside the pubsub process loop.
+	PeersForTopic(topic string) (map[peer.ID]struct{}, bool)


Hm, would a router want to limit their view of topic membership by only seeing peers that support their routing protocol? The reason I ask is because all other methods can be regarded as events, except for this one, which is pure state access. It seems to break the abstraction a bit.

I'd be more inclined to have the router and control layers be event-based, with no shared state. That could remove the need for the synthetic Eval() method -- which is only needed to protect state at an adjacent layer...

I see this model working as follows:

On startup, routers enrol with the control layer, providing a predicate that, given a []protocol.ID, returns whether the router can interact with that peer or not.

On every new peer that advertises the umbrella pubsub protocol, we call the predicate on all routers, and emit a NewPeer event to routers that signal support.

The control layer tracks which routers are handling which peers. When a peer goes away, we emit an event relevant routers.

For topic membership control messages (join or leave), the control layer emit events to the supporting routers.

Routers send messages by pushing events on a channel that the control layer is looping over.

...

This approach would insulate the state of each layer and we'd remove the need for cross-boundary scope changes.

this is a major redesign i'd rather not tackle in this pr!

Also, programming this with events makes it a lot harder to reason about; I really like the simplicity of a lock-free event loop.

Hmmm, I get that, but pubsub/gossipsub is relied on by major downstream adopters, and I'm happy to invest time and effort in that refactor. I think it would make the code more decoupled, elegant, accessible for contributors, and less spaghetti.

Isn't the lock-free event loop inherently mono-threaded, anyway? It can become a major bottleneck.

If instead we let routers keep their own state locally, and be responsible for mutating in reaction to events they receive from the control layer, I think it makes the system more functional and a lot easier to reason about.

👍 that having an Eval() function on the interface seems ridiculous (also, this being ridiculous was the reason given for #171 not being it's own router). If there's a short term need for users that don't want to fork go-libp2p-pubsub just to get their work done I'd rather be even grosser and just export the fields in PubSub so people know how unstable/temporary the state is.

A more approachable, and less intrusive, approach might be to invert a small amount of the design. For example PeersForTopic could easily be part of the PubSubRouter interface instead utilizing a PubSubControl interface. If certain structs/functions are really the same for various different routers then we could have some PubSubRouterBase struct that could be embedded in the individual routers.

raulk · 2019-10-01T12:09:08Z

pubsub.go

+
+	// Eval schedules a thunk for evaluation in the pubsub process loop.
+	// Returns false if the evaluation was aborted because the pubsub system has been shutdown.
+	Eval(thunk func()) bool


I think this is a smell. Alternative design proposal in comments above.

raulk · 2019-10-01T12:11:26Z

pubsub.go

+var ErrQueueFull = errors.New("queue full")
+
+func (p *PubSubControlView) PeersForTopic(topic string) (map[peer.ID]struct{}, bool) {
+	tmap, ok := p.topics[topic]


I'm assuming this method is on the hot path, hence the intention to avoid copying by introducing the thunk scheduling to guard state...

vyzo · 2020-03-23T10:33:23Z

closing as we decided to refactor internals before allowing out of tree routers.

vyzo added 2 commits September 23, 2019 13:50

refactor to allow external routers

1517eb3

add extData field to protobuf for external protocol implementations

94af5b1

vyzo requested review from Stebalien and raulk September 23, 2019 11:06

vyzo mentioned this pull request Sep 23, 2019

Question about implementing custom PubSubRouter #187

Open

Stebalien requested changes Sep 23, 2019

View reviewed changes

vyzo added 3 commits September 23, 2019 23:38

indent extData properly

a676455

document PubSubControl methods that are not safe to call outside the …

7aa4e4d

…event loop

export the from field in pubsub.RPC

6e5d152

vyzo requested a review from Stebalien September 23, 2019 20:50

Stebalien reviewed Sep 30, 2019

View reviewed changes

raulk requested changes Oct 1, 2019

View reviewed changes

vyzo mentioned this pull request Oct 9, 2019

add sender id to message #207

Closed

aschmahmann mentioned this pull request Dec 16, 2019

Separating Meshsub from Gossipsub #249

Closed

vyzo closed this Mar 23, 2020

Stebalien deleted the feat/external-routers branch August 6, 2024 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to allow out-of-tree router implementations #199

Refactor to allow out-of-tree router implementations #199

vyzo commented Sep 23, 2019

Stebalien left a comment

Stebalien Sep 23, 2019

vyzo Sep 23, 2019

Stebalien Sep 23, 2019

Stebalien Sep 23, 2019

vyzo Sep 23, 2019

vyzo Sep 23, 2019

Stebalien Sep 23, 2019

vyzo Sep 24, 2019

Stebalien Sep 30, 2019

raulk Oct 1, 2019

Stebalien Sep 23, 2019

Stebalien Sep 23, 2019

vyzo Sep 23, 2019

Stebalien Sep 23, 2019

vyzo Sep 23, 2019

vyzo Sep 23, 2019

Stebalien Sep 23, 2019 •

edited

Loading

vyzo Sep 23, 2019

vyzo commented Sep 23, 2019

Stebalien Sep 23, 2019

raulk Oct 1, 2019

vyzo Oct 1, 2019

raulk left a comment

raulk Oct 1, 2019

vyzo Oct 1, 2019

vyzo Oct 1, 2019

raulk Oct 1, 2019

aschmahmann Oct 2, 2019

raulk Oct 1, 2019

raulk Oct 1, 2019

vyzo commented Mar 23, 2020

Refactor to allow out-of-tree router implementations #199

Refactor to allow out-of-tree router implementations #199

Conversation

vyzo commented Sep 23, 2019

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Sep 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Mar 23, 2020

Stebalien Sep 23, 2019 •

edited

Loading