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

New builders PoC #646

Closed
wants to merge 6 commits into from
Closed

New builders PoC #646

wants to merge 6 commits into from

Conversation

Menduist
Copy link
Contributor

@Menduist Menduist commented Nov 8, 2021

#551 introduced Builders, which allows to build the switch like this:

  var switch = SwitchBuilder
    .new()
    .withRng(rng)       # Give the application RNG
    .withAddress(ma)    # Our local address(es)
    .withTcpTransport() # Use TCP as transport
    .withMplex()        # Use Mplex as muxer
    .withNoise()        # Use Noise as secure manager
    .build()

My main gripe with it is that the builders.nim file must import & handle the setup logic of every module of libp2p. Beside being fairly centralized, this also forces everyone to compile every module, even if they don't use them.

To avoid this issue with WS, I had to create a generic builder:

.withTransport(proc (upgr: Upgrade): Transport = WsTransport.new(upgr))

But now, websocket is a second class citizen, because it's harder to create than Tcp.

Other example, to implement PushIdentify, which requires some wiring up in the switch, I need to add it again in the builders.nim

This PR is an attempt to create a new builder system which alleviates some of the issues. Example build with it:

  var switch = Switch.new(peerInfo)
  switch.mount(Identify)
  switch.mount(Mplex)
  switch.mount(ConnManager)
  switch.mount(Noise, rng)
  switch.mount(TcpTransport)

Each mount is now handled in each module directly, ex in tcptransport.nim:

proc mount*[Switch](s: Switch,
  T: typedesc[TcpTransport],
  flags: set[ServerFlags] = {}) =
  
  let transp = T.new(flags, s.getUpgrade())
  s.transports &= transp

(The generic is a ugly hack to avoid circular imports in some cases)

The biggest downside of this technique is that you must mount everything manually, and in the right order (eg, mount transports last because they need the upgrade). But imo, it's easy & clear enough to add errors when you miss some mandatory protocols

Creating this PR to get some opinions. There is a lot of cleanup potential, this mount (which we can rename) could replace most constructors, the LPProtocol init method, etc

Obviously, big breaking change, which is why I would like to squeeze it before V1 if you like it

@dryajov
Copy link
Contributor

dryajov commented Nov 8, 2021

One of the reasons of having the builders in the first place was to separate object creation and initialization to avoid tight coupling, by moving it back into the transport module/class, we're back to square one.

I like the overall idea of having dedicated factory methods handling object creation and I like the potential syntax of .mount(MyProtocol) (tho I don't think the naming is right). I would go a different route and instead create a separate (outside of switch) .with(MyProtocol) method that calls the appropriate factory based on the passed type. We can even have a dynamic dispatcher that calls the correct factory that gets wired in at compile time (or maybe this is something that we can hack around with mixin). The point is that I would be careful with mixing concerns, specially when it comes to object creation which inevitably has lots of corner cases and I would keep creation and initialization out of the main protocol module/class.

@Menduist
Copy link
Contributor Author

Ok, so I did a V2.

To recap the goals:

Avoid centralization of building

Centralization, as it's done now, as a few issues:

  • A third-party can't "inject" a custom module at the middle of the building
  • The builders.nim has to import everything it wants to be able to build, so eg, nim-websock would have to be built by everyone, even if they don't use it

Impossible to get a half-built object

You should only get the usable switch once it's fully setup

Avoid tight coupling

Modules shouldn't rely on the switch, and the switch should rely on as-little modules as possible.

With that in mind, the new system:

How it's used

Creating a switch:

proc createSwitch(ma: MultiAddress, rng: ref BrHmacDrbgContext): Switch =
  var switchBuilder = SwitchBuilder.new()
  switchBuilder.with(PeerInfo, addrs=[ma])
  switchBuilder.with(MultistreamSelect)
  switchBuilder.with(Identify.new())
  switchBuilder.with(Noise, rng=rng)
  switchBuilder.with(Mplex)
  switchBuilder.with(ConnManager)
  let tcpTransport = switchBuilder.with(TcpTransport)
  switchBuilder.with(Dialer)
  
  return switchBuilder.build()

We mount each module one by one. We have multiple possibilities to mount a module: we can give only it's type, or it's type + some parameters that will be forwarded, or give an existing instance.
Each with will return the created instance, if you want to retrieve it.

Creating a module:

proc new*(
  p: typedesc[PeerInfo],
  rng = newRng(),
  addrs: openarray[MultiAddress] = []): PeerInfo
  let randomPrivateKey = PrivateKey.random(rng[])
  PeerInfo.new(randomPrivateKey.get(), addrs)
  
proc setup*[Ctx](c: Ctx,
  peerInfo: PeerInfo) =
  c.peerInfo = peerInfo

The new is like before, and will get the parameters forwarded from switchBuilder.with. The parameters should be the module configuration
The new proc setup is the "wiring" phase, where the module will get the information it needs, and install itself where it needs to.
It takes a generic Context, which could be anything, but has to handle the module needs.

A few other setups, to see what they look like:

proc setup*[Ctx](c: Ctx, i: Identify) {.raises: [Defect, LPError].} =
  i.peerInfo = c.peerInfo
  c.identify = i
  c.mount(i)
########
proc setup*[Ctx](c: Ctx, tcpTransport: TcpTransport) =
  # The upgrade is now created per-transport. I think this make sense
  # because Quic will use a different upgrader, for instance (no need for mplex)
  let upgrade = MuxedUpgrade.new(c.identify, c.muxers, c.secureManagers, c.connManager, c.ms)
  tcpTransport.upgrader = upgrade
  c.transports &= tcpTransport

With this new system, each module is completely independent, and can do his setup as he likes.

The SwitchBuilder

The SwitchBuilder adds security to this switch building phase. First of all, to call build on it, you need to have a valid Dialer setup.
Next, it will block you from setting up a module before the module's prerequisites are setup. Examples:

  var switchBuilder = SwitchBuilder.new()
  switchBuilder.with(Identify)
## Fails with: With(Identify): PeerInfo should be setup first [Defect]

  var switchBuilder = SwitchBuilder.new()
  [... setup everything ...]
  switchBuilder.with(TcpTransport)
  switchBuilder.with(Dialer)
  switchBuilder.with(WsTransport)
## Fails with: With(WsTransport): can't be done after With(Dialer) [Defect]

The SwitchBuilder remains completely generic, and will only base theses errors on the read/write of the different modules.

The main drawback of this entire system is that users have to be careful about the order they with things in. But if the order is wrong, it will fail instantly and clearly, so it's not so bad, imo.

Right now I only did the setup for the minimum amount of modules, before being sure that we want to go with this.

@dryajov

@Menduist
Copy link
Contributor Author

Menduist commented Apr 6, 2022

Closing in favor of #701 which is miles better

@Menduist Menduist closed this Apr 6, 2022
@Menduist Menduist deleted the newbuilders branch November 16, 2022 12:56
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

Successfully merging this pull request may close these issues.

2 participants