config: use fx dependency injection to construct transports

[marten-seemann]

Fixes #1210. Prerequisite for #1759 (as both the QUIC and the WebTransport transport will need to depend on a quicreuse package).

Using proper dependency injection has long been our plan to get rid of our reflection magic, which was getting more and more complicated as we added more constructor arguments.
By virtue of using fx, we now manage to construct exactly the dependencies we need. For example, when constructing a host that only uses QUIC (-based) transport(s), we don't construct an upgrader and security protocols.

There are some minor drawbacks to the new approach:

• multiplexer: it's now required to pass a fully constructed network.Multiplexer into the libp2p.Muxer constructor. This is fine, both yamux and mplex don't even have define a constructor, and I'm not aware of any other muxers. Thus, the only way the libp2p.Muxer was ever used was with a fully constructed muxer.
• security: it's now required to pass a security constructor (and not a fully constructed SecureTransport) into libp2p.Security. As far as I can tell from a cursory GitHub code search, this is the only way this API was ever used.
• transport: libp2p.Transport can only be used to construct transports if not passing any transport options (tcp.DisableReuse would be an example of a transport option). Users that want to use transport options would need to use libp2p.TransportWithOptions. This is because we're using generics in the libp2p.Transport function to preserve the type information of the option argument (see https://stackoverflow.com/questions/74327597/go-generics-infer-type-parameter-for-variadic-function-with-zero-arguments). @MarcoPolo had an idea how to refactor transport constructors to avoid splitting the function, however, this solution would require transport constructors to adopt a very peculiar format for their parameters and return values.

[marten-seemann]

I think I have a solution for the transport constructor. Will update the PR later.

[vyzo]

Good luck debugging when it doesnt work, if you even figure out it didnt.

Experience has not been overwhelmingly positive with it, only person who really (barely) understands it is @magik6k and even he doesnt like it.

Why??????

[marten-seemann]

Good luck debugging when it doesnt work, if you even figure out it didnt.

@vyzo Please don't ask how many hours went into this PR... 😭

[Wondertan]

Small 2 cents from the external dev. We have used FX for a while and had debates about it initially, but so far, it has played out pretty well. It's a learning curve for devs to understand how it works. Sometimes, it ain't easy to debug, but if you unit test builds, all the mysterious dependency graph bugs will be caught early. Also, their internal logger helps.

It is well maintained, devs are responsive, and feature requests like this are implemented quickly enough. This or a similar feature is something that the lotus team wanted to have but ended up implementing some custom wrappers around FX to replace dependencies or just to abstract away FX, afaik 🤷🏻

There is a generic-based and zero reflection do, which I am looking forward to, but it does not seem to be feature complete with FX and is less mature in general. What are alternatives you are thinking of anyway? The existing Host construction reflection code is not in a good state, IMO, and evolving to a custom DI lib does not seem like a viable solution.

[marten-seemann]

@Wondertan Thank you for sharing your experience here. I definitely agree with the steep learning curve, as I've mentioned before, way too many hours went into this PR (I had never worked with fx before).

Sometimes, it ain't easy to debug, but if you unit test builds, all the mysterious dependency graph bugs will be caught early. Also, their internal logger helps.

I was trying to build a unit test to assert that (for example) the TCP transport is not constructed when I build a host that only uses the QUIC transport. I didn't find any way to do it. Unfortunately, the event log doesn't log what is actually constructed, which makes it pretty useless. Would you be able to point me in the right direction?

[marten-seemann]

I think I have a solution for the transport constructor. Will update the PR later.

PR updated. We now only have a single transport constructor, without having to change the signatures of our transport constructors. cc @MarcoPolo

[marten-seemann]

@magik6k Would be super cool if you could take a look at that PR. Just want to check if we're doing anything crazy here...

[julian88110]

@marten-seemann this is a very good PR that improves safety, readability and debugability, thanks for making it happen!

[Stebalien]

I don't have time for an in-depth review right now, but thank you for deleting my shitty reflection code. This is an area where it's difficult to end up somewhere worse than where we started.

[MarcoPolo]

Some small changes. Overall this is a great refactor!

I think once you get it, fx is reasonable. But agree the learning curve is steep. My mental model in case it helps others:

Fx keeps track of types it has available (via fx.Supply) or types that can be provided via a call to a constructor (via fx.Provide). Fx can turn a source of type T into an parameter of type []T via the group annotation. Fx can not only match by type, but by a name annotation in cast the types are ambiguous. Ultimately fx.Invoke defines what functions are actually run, and everything else is to fulfill the dependencies of the func passed to fx.Invoke. And fx.Invoke is only run when you start the app via App.New.

Does that seem accurate to your mental model @marten-seemann ?

[MarcoPolo]

Does it make sense to annotate if the constructor does not have variadic inputs? Or maybe we should error/panic if we are passed in opts, but the constructor doesn't actually take variadic opts since this is probably a bug.

[marten-seemann]

This annotation is just annotating the result (the transport.Transport return value). I assume you meant to comment on the other annotation.

I'm ok with returning an error then, but this only covers a small part of the error space. For example, the user could still give us the QUIC constructor and a TCP option. I think fx will give us an error then.

Thinking about it, we should have a test for that! I'll add one.

[marten-seemann]

I think fx will give us an error then.

It doesn't. The argument is just not getting used. We'd have to use some reflections ourselves to make sure that the argument types match.

[marten-seemann]

I implemented a check (see last commit). Unfortunately, this adds some more reflection code, but I don't think it's too bad. And it's limited to the one libp2p.Transport function.

[MarcoPolo]

This annotation is just annotating the result (the transport.Transport return value). I assume you meant to comment on the other annotation.

Am I wrong? I'm pointing out that We are annotating the fx.Supply of the opt param with the resultTags tag. Not the result of the the transport constructor (transpor.Transport) in line 164.

[marten-seemann]

This annotation is just annotating the result (the transport.Transport return value). I assume you meant to comment on the other annotation.

Oh yes, my comment doesn't make any sense. Sorry!

I added the check that you suggested, including a test.

[vyzo]

Some more contrarian views -- note that I am not opposing this pr per se, just trying to raise awareness of the issues it will create and question whether really this is the best (or even just good!!!!) alternative.

Brace yourself for a rant.

The old reflection code, shitty as it was, had one great advantage: it was right in front of our eyes and easy to understand. Anyone diving could see how our dependency resolution mechanism worked, and quickly make changes (modulo messy stuff).

Now this fx.... Oh boy. I have been working with in lotus for quite a while now, and I am telling you it is sh*t. And every now and then it breaks in really ugly ways, and you can't even tell it broke -- I discovered some brokage by network packets and it was very subtle and bad with the bootstrappers.
And don't get me started on the error messages and the general wtf did I do wrong thing when it breaks.

Really, this whole thing is simply a glamorous HACK. This is not quality software, and this is not to mock or belittle the engineers that work on it. Golang is just not conductive to this kind of software, and they might be doing the best they can. But the thing is just no good for your sanity.

So that brings us to this pr: we end up with a system that is barely understandable, hard to debug, and make it prohibitively expensive for new contributors to actually understand wtf is going on when they invoke libp2p.New.

And really, my biggest gripe is that I DONT KNOW IF IT REALLY WORKED when it gives a constructed object.

You have been warned.

[MarcoPolo]

I think this is an improvement to what we had before, let's ship it!

That said, I'll play around with do a bit. I think even if we migrate to do, the migration is a lot easier from fx -> do than it is from magic reflection -> do, as we'll have to do a lot of the same things here.

[guseggert]

Kubo uses fx extensively so I can share our experience with it.

Disclaimer: I don't like DI frameworks in general, mainly for the reason the @vyzo enumerated...they operate as black boxes on arguably some of those most critical logic in your codebase--the wiring of dependencies. They're hard to verify correct behavior and when things break, your life sucks because there's no clear stack trace or code path to follow.

That said, they do have the advantage of imposing a common language for talking about how to wire dependencies, instead of having to invent some bespoke one for each application. This leads to less code to write and maintain, folks can get answers from Google on their dependency wiring questions, and you get a whole lot of dependency-wiring functionality for free (lifecycle management, different ways of referring to dependencies such as by type or name, cross-cutting or surgical operations on the dependency graph, cycle detection, etc.). Also if there are multiple applications/libs interacting and they all speak the same language (use the same DI framework), that can get pretty convenient.

As for Kubo:

We recently released an fx plugin to let folks customize the Kubo dependency graph, so they can inject their own instance of core interfaces, customize libp2p options, etc. I think we all agreed that ideally we'd remove fx and provide some bespoke way to do this, but we couldn't because it's a lot of effort that we have no chance of prioritizing. Which to me drives home its main benefit--it took me a day to add an fx plugin, and from that users automatically got complete customizability of the dependency graph, instead of spending weeks reworking the Kubo wirings and then only getting whatever kind of customizability our bespoke API would allow.

There are some things we don't like about our fx config:

• We use a lot of nested fx.Options which makes them hard to customize after-the-fact
  • The fx docs call this out as an anti-pattern: // Use this pattern sparingly, since it limits the user's ability to customize their application.
• We have some anonymous, unnamed lifecycle hooks, which are hard to customize...name them or use exported types so you have some way of referring to them externally
• We use value groups, e.g. libp2p options, which again makes it hard to customize since they don't have names

And more generally, since DI frameworks don't force you to deal directly with your dependency graph, they make it easy to accrete complexity and grow into mudballs.

If I could do fx over in Kubo, I'd definitely not use those features above. Generally I'd just avoid any "advanced", non-trivial feature of any DI framework. The fancier they are, the harder they are to understand and use correctly.

[lthibault]

I'm late to the party, but thought I'd share my notes on Fx anyway.

We're quite happy with our use of Fx in Wetware, and I think this is largely because we have been very careful to keep Fx out of library code. Currently, we enforce a boundary between application code (internal/...) and library code (pkg/...). Fx is only allowed in the former (with the trivial exception of embedding fx.In in certain config structs in the library code).

This has given us the benefit of making the wiring of library types more legible in the application, while also avoiding DI creep. I'm generally of the opinion that our use of Fx is an indicator (rather than the cause) of accidental complexity in our application. In other words, the only thing worse than DI is the boilerplate code it replaces.

I'll also note that writing a custom logger for Fx has done a great deal to facilitate debugging.

While I generally share @vyzo's outlook on DI frameworks, I've been pleasantly surprised by this one.

[marten-seemann]

@lthibault Thank you for sharing your experience!