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

Replace chain store's headEvents "pubsub" with a synchronous event bus #2309

Closed
anorth opened this issue Mar 18, 2019 · 2 comments
Closed

Replace chain store's headEvents "pubsub" with a synchronous event bus #2309

anorth opened this issue Mar 18, 2019 · 2 comments

Comments

@anorth
Copy link
Member

anorth commented Mar 18, 2019

Description

The chain store carries a headEvents reference which is a channel-based "pubsub" object from https://github.com/cskr/pubsub. The object supports notification of when a new head tipset is discovered.

The asynchrony forced by using channels complicates, things, and in particular has led to a mutable HeaviestTipSetHandled callback func on the Node purely so that node.TestUpdateMessagePool can tell when the subscriber to that pubsub has finished.

Forcing the use of channels here unnecessary. This use would be much better served by a simple, synchronous event bus like this one. The message pool test could then simply update the head and know that the node has responded when the event post returns.

Note that this pattern doesn't prevent asynchrony for any particular event/subscriber, but makes it opt-in. Reducing unnecessary concurrency, and scoping it tightly, could also help us avoid work and/or headaches of handing concurrency in objects that are transitively reachable from any goroutine (we almost certainly don't handle this properly at present).

Since an event bus is a relatively simple pattern, I suggest we write our own, a simpler version of that linked above. We can then limit & customise functionality to exactly our needs. I expect an event bus will be useful for more than this single use case.

Acceptance criteria

  • New heaviest tipsets are notified via a synchronous event bus
  • Node.HeaviestTipSetHandled is removed
  • github.com/cskr/pubsub is removed from dependencies

Risks + pitfalls

  • This will involve a little refactor of the current tipset handling code.
  • The referenced event bus does not plumb a Context, but we should

Where to begin

In node.go, understand usage of HeaviestTipSetCh and HeaviestTipSetHandled.

@anorth anorth added C-technical-debt help wanted Call for participation: More complex than good-first-issue labels Mar 18, 2019
@anorth
Copy link
Member Author

anorth commented Jun 23, 2019

Libp2p have produced a local event bus, see libp2p/go-libp2p#653. On my first reading, it seems to involve mandatory channels (and hence asynchrony), but worth investigating.

@anorth
Copy link
Member Author

anorth commented Jul 30, 2019

The ugly HeaviestTipSetHandled callback has since been purged, but the forced concurrency here is still undesirable.

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

No branches or pull requests

2 participants