nethermind icon indicating copy to clipboard operation
nethermind copied to clipboard

Feature/automatic dependency injection 1

Open asdacap opened this issue 2 years ago • 5 comments

  • Third attempt at adding automatic dependency injection.
  • This one start from before IStep.
  • Plugins now register Autofac.IModule, which is configured before any step is run. Some plugin interface was removed as its no longer needed.
  • IStep are now resolved via DI. So the plan is to migrate the steps in order of execution. This should eventually touch every component.

But why?

  • Cleaner plugins. No need to add manual hooks. These hooks, factories and such can be done automatically. No need to add manual hooks for each use case.
    • Eg: Replace IVirtualMachine without creating an explicit hook, somewhere, or caring about dependency order.
    • Eg: This #6476, probably.
  • More flexible initialization.
    • Eg: If i want to create a command that start VerifyTrie without starting anything else, it will automatically start only what is needed.
    • Eg: If I want to start integration test for this particular part of code, I don't have to re-code how to wire them all up.
    • Eg: If I want to re-start BlockTree for whatever reason, the lifecycle is much more clearly defined, so its a lot easier to achieve.
  • Far less boilerplate (eventually).
  • Automatically determine component dependencies, recursively. On missing deendency, it will throw.
  • Automatically dispose components that implement IDispose. No longer miss dispose, probably.

Why AutoFac instead of .net core's Microsoft.Extensions.DependencyInjection?

  • Mainly due to clearer separation lifetime. Core's DI, have transient, scoped and no-scoped, which is likely the only thing you need with a web server. Maybe its possible to have more nested lifetime, but I can't find doc for it or its hard to find doc for it. AutoFac have clear doc on how to create child lifetime recursively, which can be useful for things like world state lifetime, rpc module lifetime with inner world state lifetime, etc.
  • AutoFac's IModule, which has a clearer intent IMO.
  • Much clearer doc IMO. Most of Microsoft.Extensions.DependencyInjection is in context of ASP.Net.
  • Have more feature. Probably a bad thing, probably not. Since I can dynamically resolve IConfig, its probably a good thing. who knows.

Changes

  • INethermindPlugin now resolved via dependency injection on a small pre-start container which have access to config.
  • Added INethermindPlugin.Enabled which should return true if the plugin is enabled. Plugin is expected to use config to know if it is enabled or not. Disabled plugin will not have its Module loaded.
  • Added INethermindPlugin.Module which can return an Autofac IModule to replace arbitrary component. Plugin that does not replace or declare anything yet, can just return null. Really... this is all a plugin usually need.
  • Remove IConsensusPlugin.GetApi. Plugins now replace INethermindApi via its module.
  • Remove IInitializationPlugin. Plugins now declare that it should run step, by actually declaring IStep from its module.
  • Removed the previous special IStep plugin discovery thing that depends on having the same INethermindApi implementation.
  • IStep is now is resolved from the DI container. Previous handling to make sure overriding IStep from plugin take priority is kept.
  • Removed InitializeNodeStat replaced with NetworkModule.
  • Merged FilterBootNodes, UpdateDiscoveryConfig into SetupKeyStore. Most code in SetupKeyStore moved to KeyStoreModule.

Types of changes

What types of changes does your code introduce?

  • [X] Refactoring

Testing

Requires testing

  • [X] Yes
  • [ ] No

If yes, did you write tests?

  • [X] Yes
  • [ ] No

Notes on testing

  • Seems to work.
  • Running hive....

asdacap avatar Jan 09 '24 06:01 asdacap

I thought container would replace INethermindApi alltogether?

This needs also a lot of testing (AuRa + Optimism too).

Eventually, yes.

asdacap avatar Jan 10 '24 23:01 asdacap

@asdacap what do you think about something like this: A hybrid approach till we switch everything bit by bit to utilize DI https://github.com/NethermindEth/nethermind/blob/refactor%2Fintroduce_dependency_injection/src%2FNethermind%2FNethermind.Api%2FNethermindApiCustomResolver%20.cs Dont mind the implementation, it could probably be done in a better way, but the idea could stand?

smartprogrammer93 avatar Jan 11 '24 00:01 smartprogrammer93

@asdacap what do you think about something like this: A hybrid approach till we switch everything bit by bit to utilize DI https://github.com/NethermindEth/nethermind/blob/refactor%2Fintroduce_dependency_injection/src%2FNethermind%2FNethermind.Api%2FNethermindApiCustomResolver%20.cs Dont mind the implementation, it could probably be done in a better way, but the idea could stand?

You have a point. This would make sense if we are migrating something from the middle of the dependency stack where its dependency is not migrated yet, or new code that wants to immediately use dependency injection without waiting for its dependency to be migrated.

Hard to know exactly. I'm thinking of migrating things from bottom up, so there should not be a need for this except for new code. If guess if there is demand for it.

asdacap avatar Jan 11 '24 00:01 asdacap

Can you layout the reason why autofac instead of Microsoft's Engine. I feel like Autofac is a bit more complicated. Is it justified?

smartprogrammer93 avatar Jan 11 '24 01:01 smartprogrammer93

Can you layout the reason why autofac instead of Microsoft's Engine. I feel like Autofac is a bit more complicated. Is it justified?

Added reasoning.

asdacap avatar Jan 11 '24 01:01 asdacap