-
Notifications
You must be signed in to change notification settings - Fork 434
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
Refactor/More proper use of DI for Synchronizer #7500
Merged
Merged
Changes from 44 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
4df93ed
Seems nice
asdacap 4b3cc69
Snap provider
asdacap 479a3f8
Sync dispatcher
asdacap 0a52825
Dedup
asdacap 3144f3b
Multi sync mode selector
asdacap c85f3e8
Fast and full sync
asdacap c126f95
The merge synchronizer
asdacap 5d21826
Use reflection
asdacap 3d90236
Fic test
asdacap b9dac97
Minor cleanup
asdacap 0f5343d
Remote block downloader
asdacap 1181068
Reduce constructor parameter
asdacap 54facc1
Add unit tests
asdacap 67fe004
Cleanup
asdacap aecaea1
Whitespace
asdacap 867f1f3
Pass through the servide collection
asdacap cb82a56
Use a need to wait for header config
asdacap 7b2b7ec
Remove more manual construction
asdacap 13e167f
Whitespace
asdacap 7ba6845
Merge remote-tracking branch 'origin/master' into feature/minor-di
asdacap 9b0cf3d
Do this more properly
asdacap b06b200
Set sync mode selector from container directly
asdacap 8034622
Get component directly from container
asdacap 915047e
Remove unnecessary code
asdacap e4b3baa
I thought I specifically fixed this before
asdacap fd8845e
Merge remote-tracking branch 'origin/master' into feature/minor-di-pr…
asdacap 1c54c1d
Well.. might as well make everything autofac
asdacap e7abfba
Fix test
asdacap 0b58c9c
Cleanup
asdacap a90fb3f
Why did you not fail earlier?
asdacap c14ae8b
Use module. Module nice.
asdacap 0f73b6b
Fix fast sync being loaded in tests
asdacap dd015d3
Merge remote-tracking branch 'origin/master' into feature/minor-di-pr…
asdacap ee4cf91
Fic build
asdacap 69bcc78
Merge branch 'fix/build-on-nodeloaders-test' into feature/minor-di-pr…
asdacap dfd0f34
Merge remote-tracking branch 'origin/master' into feature/minor-di-pr…
asdacap f8612f4
Merge remote-tracking branch 'origin/master' into feature/minor-di-pr…
asdacap 399fe76
Update src/Nethermind/Nethermind.Api/IApiWithNetwork.cs
asdacap 2083c73
Update src/Nethermind/Nethermind.Db/IDbProvider.cs
asdacap 101b779
Update src/Nethermind/Nethermind.Db/IDbProvider.cs
asdacap 54680c7
Address comment
asdacap c0dd797
Update src/Nethermind/Nethermind.Api/IApiWithBlockchain.cs
asdacap 659cc19
Merge remote-tracking branch 'origin/master' into feature/minor-di-pr…
asdacap 0e292fb
Merge remote-tracking branch 'origin/feature/minor-di-properer' into …
asdacap d95e72c
Merge remote-tracking branch 'origin/master' into feature/minor-di-pr…
asdacap File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
113 changes: 113 additions & 0 deletions
113
src/Nethermind/Nethermind.Core.Test/ContainerBuilderExtensionsTests.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
// SPDX-FileCopyrightText: 2024 Demerzel Solutions Limited | ||
// SPDX-License-Identifier: LGPL-3.0-only | ||
|
||
using System; | ||
using Autofac; | ||
using FluentAssertions; | ||
using NUnit.Framework; | ||
|
||
namespace Nethermind.Core.Test; | ||
|
||
public class ContainerBuilderExtensionsTests | ||
{ | ||
[Test] | ||
public void AddPropertiesFrom_CanAddProperties() | ||
{ | ||
ITestInterface interfaceImplementation = new InterfaceImplementation(); | ||
IContainer sp = new ContainerBuilder() | ||
.AddPropertiesFrom(interfaceImplementation) | ||
.Build(); | ||
|
||
sp.ResolveOptional<DeclaredService>().Should().NotBeNull(); | ||
sp.ResolveOptional<DeclaredInBase>().Should().BeNull(); | ||
sp.ResolveOptional<Ignored>().Should().BeNull(); | ||
sp.ResolveOptional<DeclaredButNullService>().Should().BeNull(); | ||
} | ||
|
||
[Test] | ||
public void TestRegisterNamedComponent() | ||
{ | ||
IContainer sp = new ContainerBuilder() | ||
.AddScoped<MainComponent>() | ||
.AddScoped<MainComponentDependency>() | ||
.RegisterNamedComponentInItsOwnLifetime<MainComponent>("custom", cfg => | ||
{ | ||
// Override it in custom | ||
cfg.AddScoped<MainComponentDependency, MainComponentDependencySubClass>(); | ||
}) | ||
.Build(); | ||
|
||
using (ILifetimeScope scope = sp.BeginLifetimeScope()) | ||
{ | ||
scope.Resolve<MainComponent>().Property.Should().BeOfType<MainComponentDependency>(); | ||
} | ||
|
||
MainComponentDependency customMainComponentDependency = sp.ResolveNamed<MainComponent>("custom").Property; | ||
sp.ResolveNamed<MainComponent>("custom").Property.Should().BeOfType<MainComponentDependencySubClass>(); | ||
|
||
sp.Dispose(); | ||
|
||
customMainComponentDependency.WasDisposed.Should().BeTrue(); | ||
} | ||
|
||
private class MainComponent(MainComponentDependency mainComponentDependency, ILifetimeScope scope) : IDisposable | ||
{ | ||
public MainComponentDependency Property => mainComponentDependency; | ||
|
||
public void Dispose() | ||
{ | ||
scope.Dispose(); | ||
} | ||
} | ||
|
||
private class MainComponentDependency : IDisposable | ||
{ | ||
public bool WasDisposed { get; set; } | ||
|
||
public void Dispose() | ||
{ | ||
WasDisposed = true; | ||
} | ||
} | ||
|
||
private class MainComponentDependencySubClass : MainComponentDependency | ||
{ | ||
} | ||
|
||
private class InterfaceImplementation : ITestInterface | ||
{ | ||
public DeclaredService TheService { get; set; } = new DeclaredService(); | ||
public DeclaredButNullService? NullService { get; set; } = null; | ||
public Ignored IgnoredService { get; set; } = new Ignored(); | ||
public DeclaredInBase BaseService { get; set; } = new DeclaredInBase(); | ||
} | ||
|
||
private interface ITestInterface : ITestInterfaceBase | ||
{ | ||
DeclaredService TheService { get; set; } | ||
DeclaredButNullService? NullService { get; set; } | ||
|
||
[SkipServiceCollection] | ||
Ignored IgnoredService { get; set; } | ||
} | ||
|
||
private interface ITestInterfaceBase | ||
{ | ||
DeclaredInBase BaseService { get; set; } | ||
} | ||
|
||
private class DeclaredInBase { } | ||
private class DeclaredService { } | ||
private class DeclaredButNullService { } | ||
private class Ignored { } | ||
|
||
private class DisposableService : IDisposable | ||
{ | ||
public bool WasDisposed { get; set; } = false; | ||
|
||
public void Dispose() | ||
{ | ||
WasDisposed = true; | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Autofac needed on top of Microsoft.Extensions.DependencyInjection? If because of modules - can it be replicated by using separate
IServiceBuilder
s instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be my question as well. I believe
Autofac
provides named registrations whileMicrosoft.Extensions.DependencyInjection
may lack it. Not sure if this is the case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong.
ServiceKey
etc are the way to go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For named registrations, there's
IKeyedServiceProvider
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as outlined here #6483.
I originally wanted to stick with
Microsoft.Extensions.DependencyInjection
, but can't make it work. See #7485.