Reduce use of globals in SiloProviderRuntime #2485
Conversation
I'm on board, I think is the way to go, but @sergeybykov did push back on this last time as it would be a breaking change |
| { | ||
| services.TryAddSingleton(sp => new GrainCreator(null, sp.GetRequiredService<Func<IGrainRuntime>>())); | ||
| } | ||
| services.TryAddSingleton<GrainCreator>(); |
There was a problem hiding this comment.
I'm trying to understand if this is correct. Today we explicitly pass null as a first argument to new GrainCreator, which triggers it to use Activator.CreateInstance(type) for instantiating grain activations. This will make it always use ActivatorUtilities.CreateFactory(type, Type.EmptyTypes).
Why do we need to change this? Seems unrelated to the stated purpose of the PR.
There was a problem hiding this comment.
I made a note about this above (you can see @jdom's comment on this PR).
This is needed because some tests require access to things which were previously static but are now no longer exposed in a static manner. Therefore, we inject them into the grains used by the tests. The issue is that under the existing setup, constructor injection is not enabled for grains unless the user specifies a startup class - which is weird because even if we don't actually inject anything in our startup class, it enables dependency injection. It's a violation of the principle of least surprise.
| { | ||
| get { return appLogger; } | ||
| } | ||
| public Logger AppLogger => appLogger; |
There was a problem hiding this comment.
In the future, it would help to group such non-essential changes in a separate commit to simplify reviewing. It's pretty easy to do with SourceTree.
There was a problem hiding this comment.
Yep, sorry. I'll shake the habit.
| : base(Constants.TypeManagerId, myAddr) | ||
| { | ||
| if (implicitStreamSubscriberTable == null) throw new ArgumentNullException(nameof(implicitStreamSubscriberTable)); |
There was a problem hiding this comment.
A minor thing, but this moved throwing of an ANE to the constructor TypeManager while it used to happen only when GetImplicitStreamSubscriberTable was called. Today this won't break because the table happens to get initialized inside StreamingInitialize() before TypeManager is instantiated. But this may easily break in the future if we rearrange things or make some of the streaming components pluggable and maybe even optional.
There was a problem hiding this comment.
That's true. The NRE is not really needed here. The value won't be null even if it hasn't yet been initialized - DI is used to construct ImplicitStreamSubscriberTable and then TypeManager is constructed using it inside Silo.CreateSystemTargets like so:
var implicitStreamSubscriberTable = Services.GetRequiredService<ImplicitStreamSubscriberTable>();
this.RegisterSystemTarget(new TypeManager(this.SiloAddress, this.typeManager, implicitStreamSubscriberTable));Currently, it's initialized in Silo.DoStart before the call to CreateSystemTargets,
On the client side, it's initialized by calling into the gateway (which hits the TypeManager system target).
So I believe this is sound
There was a problem hiding this comment.
In the latest revision, you'll see that this aligns to what Ben is doing since his heterogenous silos PR
ReubenBond
force-pushed
the
statics-removal
branch
2 times, most recently
from
21c90ee
to
55cc148
Compare
| @@ -820,5 +790,89 @@ public void BreakOutstandingMessagesToDeadSilo(SiloAddress deadSilo) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| public StreamDirectory GetStreamDirectory() | |||
| { | |||
There was a problem hiding this comment.
These are originally from SiloProviderRuntime
ReubenBond
force-pushed
the
statics-removal
branch
from
915ae23
to
e15a3da
Compare
|
Addressed all feedback, functionals should all be passing - 2 were failing for the same reason (multiple constructors), but they're fixed. EDIT: btw, I undid most of the superfluous/stylistic changes you pointed out @sergeybykov |
ReubenBond
force-pushed
the
statics-removal
branch
from
e15a3da
to
afe16a5
Compare
Oh, sorry, I didn't mean to revert them. Cleanup changes like that are good. It's just easier to review when such changes are in a separate commit. If you still have them around and can easily submit, please do. |
SiloProviderRuntime.InstanceglobalISiloRuntimeClient, which contains methods moved fromSiloProviderRuntime.GrainCreatornow always usesActivatorUtilities.CreateFactoryinstead ofActivator.CreateInstance, meaning that grain classes with multiple public constructors cannot be constructed unless they are explicitly registered. @jdom are you fine with this? This behavior is consistent with ASP.NET as far as I know.SiloProviderRuntimeare now forwarded toInsideRuntimeClientwhere I think they are a more natural fit. We can move them elsewhere if desired.This is working towards #467. The idea is to make multiple smaller commits to remove globals so that we can implement support for multiple distinct grain clients per AppDomain.