Isolate service information computed in child scopes from the parent scope

[alistairjevans]

This fixes #1324, and allows an assembly context loaded inside a lifetime scope to be unloaded after the lifetime scope is disposed, without disposing of the container. I've tested that this is actually true with an assembly-unload-test-harness of sorts.

These changes still require the shared ReflectionCacheSet to be cleared at the end of the scope, but I've got a test method for creating a lifetime scope for the context that handles this fairly readily:

static class AlcScopeCreationExtensions
{
    public static ILifetimeScope BeginLifetimeScopeForAssemblyLoadContext(
        this ILifetimeScope currentScope, 
        AssemblyLoadContext loadContext, 
        Action<ContainerBuilder> config)
    {
        if (loadContext == AssemblyLoadContext.Default)
        {
            throw new InvalidOperationException($"Only use {nameof(BeginLifetimeScopeForAssemblyLoadContext)} for non-default load contexts");
        }

        var newScope = currentScope.BeginLifetimeScope(config);

        newScope.CurrentScopeEnding += (sender, args) =>
        {
            var alcAssemblies = loadContext.Assemblies;

            // Clear the reflection cache when the inner scope goes away.
            ReflectionCacheSet.Shared.Clear((assembly, type) =>
            {
                return alcAssemblies.Contains(assembly);
            });
        };

        return newScope;
    }
}

Note that I have not added that method in my current changes, although there is an argument for it, more on that in a moment.

Raising this as draft for two reasons; first of all, I need to add some tests, and secondly, there are 'some' performance implications for this change that mean it may need consideration/tweaking before it goes out.

---

What have I done?

When the ExternalRegistrySource asks a parent registry "what are the registrations for this service", it wraps the Service in a ScopeIsolatedService instance before sending it to the parent registry.

When evaluating the service information for a service wrapped in ScopeIsolatedService, if the process of evaluating the service results in no registrations for that service, the DefaultRegisteredServicesTracker does not keep the reference to the service information, discarding it from the dictionary of all service info at the end of the initialisation process.

This does cause a slight annoyance with CollectionRegistrationSource, where that source always returns a registration for the type, even if actually evaluating it would return an empty set.

To address that, there is a new interface IPerScopeRegistrationSource, applied to CollectionRegistrationSource, that indicates it is added as a source to all new scopes, so that the creation of that registration happens in the local scope; sources marked with IPerScopeRegistrationSource are also never invoked when a ScopeIsolatedService is being evaluated in the parent services tracker.

To try and address the performance overheads of not having the single CollectionRegistrationSource, I have pushed the generated factory methods into the reflection cache.

Perf Impact / Opt-in?

This change will have some performance impact if someone is frequently creating a new scope with it's own registry, and requesting a service that doesn't exist in any parent registry, e.g.

using (var requestScope = _container.BeginLifetimeScope("request", b => b.RegisterType<C1>()))
{
  requestScope.Resolve<NeverRegistered>();
}

This is because each scope above the requestScope would basically have to do service initialisation again each time it's resolved. Not ideal, certainly.

So...the question then is whether that's ok (for simplicity reasons), or we make people opt in to this behaviour where we wrap a service in ScopeIsolatedService, e.g. the example I have above, BeginLifetimeScopeForAssemblyLoadContext.

[codecov]

Codecov Report

Base: 78.12% // Head: 78.53% // Increases project coverage by +0.40% 🎉

Coverage data is based on head (430e28b) compared to base (2adc678).
Patch coverage: 97.56% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1350      +/-   ##
===========================================
+ Coverage    78.12%   78.53%   +0.40%     
===========================================
  Files          197      199       +2     
  Lines         5636     5715      +79     
  Branches      1149     1161      +12     
===========================================
+ Hits          4403     4488      +85     
+ Misses         717      715       -2     
+ Partials       516      512       -4     

Impacted Files	Coverage Δ
...thing/AnyConcreteTypeNotAlreadyRegisteredSource.cs	86.66% <ø> (+3.33%)	⬆️
src/Autofac/Core/Container.cs	67.74% <50.00%> (-1.23%)	⬇️
src/Autofac/Core/ScopeIsolatedService.cs	80.00% <80.00%> (ø)
...utofac/Core/Registration/ExternalRegistrySource.cs	91.66% <87.50%> (+5.95%)	⬆️
src/Autofac/Core/Lifetime/LifetimeScope.cs	84.93% <100.00%> (+2.99%)	⬆️
...e/Registration/DefaultRegisteredServicesTracker.cs	86.20% <100.00%> (+1.53%)	⬆️
...tration/ExternalRegistryServiceMiddlewareSource.cs	88.88% <100.00%> (+8.88%)	⬆️
...tofac/Core/Registration/ServiceRegistrationInfo.cs	77.77% <100.00%> (+0.41%)	⬆️
...atures/Collections/CollectionRegistrationSource.cs	94.31% <100.00%> (+1.07%)	⬆️
...ac/Util/Cache/ReflectionCacheAssemblyDictionary.cs	70.00% <100.00%> (+7.50%)	⬆️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tillig]

On first glance I don't think this looks too complicated. I do worry a little about the allocations in wrapping those services in a special decorator during query, but maybe I shouldn't worry so much about it. What other registration sources would potentially need the IPerScopeRegistrationSource? I'm curious if that's something we'd need to expose at the outset or not (e.g., maybe it's an internal interface to start?).

Re requestScope.Resolve<NeverRegistered>(); isn't that going to be expensive anyway because it throws?

How does ACTNARS get affected here? That thing has always been a thorn in my side.

I can't say I 100% grok the whole of it yet; I'm swamped on some other day-job stuff so my mind is in PowerShell-module-land rather than nuances-of-Autofac-land. It'll take me a couple more read-throughs to really get it.

[alistairjevans]

I think my example on perf-impact concern was a bit sub-par; of course, if the type doesn't exist anywhere it'll throw, so not an issue. However, if a service is defined in the child scope, but in no parent, each parent will get asked about the service in each new child scope, and will not have a cached "no registrations" service info. E.g.

var builder = new ContainerBuilder();

var container = builder.Build();

using (var outerScope = _container.BeginLifetimeScope(builder => {}))
{
    var scopeCount = 0
    while (scopeCount < 100)
    {
        using (var requestScope = outerScope.BeginLifetimeScope(builder => builder.RegisterType<MyService>()))
        {
            requestScope.Resolve<MyService>();    
        }

        scopeCount++;
    }
}

In the above example, each time requestScope.Resolve<MyService>() is called, it will consult the tracker in requestScope, which will check the tracker in outerScope via ExternalRegistrySource.

In the current code, the outerScope will evaluate whether it can provide MyService (including running all the registration sources, and asking the container tracker), then remember that fact.

In the new code, the outerScope will evaluate whether it can provide MyService, notice that no registrations were found, and discard that fact.

Thus, each time MyService is resolved, all the parent scopes above it will have more work to do. So, outerTracker will evaluate its sources 100 times.

I'm a little concerned that's going to be too high a price to not have this isolation gated behind an opt-in.

I also think the concern over allocating an additional IsolatedService wrapper is valid, if we can avoid it.

I'm continuing to mull options for this that don't require opt-in.

[tillig]

That makes sense... and it all sounds expensive. 💸

Without looking at the code personally... what if the dependency for caching was inverted? The child scope queries the parent to see if it can fulfill the service, but instead of the parent caching that info, the child does - then it knows to bypass that query entirely on the next request. Again, I have totally not looked at the code and I'm on my phone right now, but it seems like something should cache it, and if it's not the parent...🤷‍♂️

[alistairjevans]

Well, the caching by the child scope is basically what happens already.

Only the parent scopes see IsolatedService; the originating child scope already caches the output of the parents by remembering that the ExternalRegistrySource returned nothing and not asking.

Problem is, that child scope could last for just the lifetime of a request and then be discarded, losing that cached info.

[tillig]

Okay, I'm with you now. Hmm. ☹️

[alistairjevans]

One nice thing about using an opt-in approach using (for example) BeginIsolatedLifetimeScope, is that the caching sort of naturally "works", provided people are sensible about which scopes they isolate.

For example:

var builder = new ContainerBuilder();

var container = builder.Build();

using (var isolatedScope = _container.BeginIsolatedLifetimeScope(builder => 
{ 
  // Load some plugin assemblies, whatever is required.
}))
{
    using (var requestScope = outerScope.BeginLifetimeScope(builder => builder.RegisterType<MyService>()))
    {
        requestScope.Resolve<MyService>();    
    }
}

In this model, external registry queries from isolatedScope up to the container are cached in isolatedScope, and any resolves in scopes nested inside isolatedScope are treated as currently, with empty results cached at isolatedScope as well.

As long as the isolatedScope isn't a per-request scope, it all basically hangs together. The guidance would have to be, don't load plugins on a per-request basis, which frankly seems like a sensible recommendation anyway.

[tillig]

Seems reasonable. I anticipate the potential desire to replace the root lifetime for a web app coming from this next. However, I feel like that'd potentially leave the app in an inconsistent state...

... Which is the whole reason we didn't keep mutable containers in the first place, so I'd be inclined to deny that request, should it arise.

[alistairjevans]

I spent a bunch of time this morning thinking about the name of an ILifetimeScope method that would accurately represent what this new method was for, and I just couldn't escape the fact that BeginIsolatedLifetimeScope was far too prone to mis-use and misunderstanding ("oh, maybe this means none of my parent scope types are included in IEnumerable<T> -> now my performance is bad". Just applying it would make performance drop if you didn't need the functionality it provides.

So, I think the best option is to actually associate the method to its purpose, that is, to support the use of AssemblyLoadContext within Autofac, and allow said context to be unloaded. That really is the only real use-case for such a method, so it feels less prone to mis-use if we actually say that's what it's for. To that end, I've defined:

ILifetimeScope BeginLoadContextLifetimeScope(AssemblyLoadContext loadContext, Action<ContainerBuilder> configurationAction);

ILifetimeScope BeginLoadContextLifetimeScope(object tag, AssemblyLoadContext loadContext, Action<ContainerBuilder> configurationAction);

By requiring the user to provide an AssemblyLoadContext, it forces them to think that they only need this when they are working with those load contexts. It also allows us to automatically handle the clean-up of the ReflectionCache so the loadContext will be unloadable when the scope is disposed.

The addition of methods it ILifetimeScope make this an actual breaking change I'd suggest.

[tillig]

Good call on tying the load/unload style of lifetime scope to the assembly load context. It definitely makes it clearer, and I agree on the improvement in naming. I also like the check for AssemblyLoadContext.Default to make sure folks aren't basically opting into the perf hit and never actually unloading anything.

[tillig]

A couple questions/comments in there, mostly around documentation and helping folks use the feature.

[tillig]

It seems like there's a gotcha around HasCustomServiceMiddleware such that if it has custom middleware it may not be able to be unloaded (see DefaultRegisteredSerivcesTracker). It may be worth mentioning that here, or possibly in documentation around the feature. That won't be immediately obvious.

[alistairjevans]

I think custom middleware from a loaded assembly context should work just fine...in that once the scoped registry is disposed, the custom middleware reference should be lost and the context becomes unloadable. I'll add a test for that too.

[tillig]

Looking good. I think there are just a couple of tests remaining.

[alistairjevans]

Right, sorry this has taken longer than I expected, but I ran into a bit of a problem when running the IEnumerable<T> test.

Basically, when disposing of a "load context aware" lifetime scope, and checking the assembly of each cache entry key to determine whether or not the entry needs to be removed from the cache, it turned out (seems obvious with hindsight), that IEnumerable<TypeLoadedFromExternalAssembly> has an assembly of Private.CoreLib, with no knowledge of the assembly for TypeLoadedFromExternalAssembly. So, with the previous code, resolving IEnumerable<TypeLoadedFromExternalAssembly> means that the loaded context would not unload after the scope was disposed.

Seeking a generic solution to this problem, I implemented TypeAssemblyReferenceProvider, which seeks to search a given type's generic arguments, element types, base types, etc, for all referenced assemblies.

This works just fine (have added some separate tests for TypeAssemblyReferenceProvider, please say if you think there are cases I have missed), but we should probably make a note in our docs that disposing of lifetime scope created with BeginLoadContextLifetimeScope is a CPU-intensive operation as we ensure that the provided context is definitely unloadable once disposal has completed.

[alistairjevans]

Had to amend the build to ensure we build our LoadContext scenario project first before running tests, because we intentionally don't reference the project directly, so it won't be built automatically.

[tillig]

Looking better. A couple of comments about duplicate code or whatever, but I think this is super close.

I just thought of another test, and I don't know if it indicates I'm not up on how type references are held in an assembly load context, but... transitive references.

So let's say you have two assemblies.

• Assembly 1 has the type A.Service1 in it.
• Assembly 2 has the type B.Service2 in it.
• A.Service1 has a property public B.Service2 MyService {get; private set;} but that property is not registered with Autofac.
• A.Service1 has a method public void SetService(B.Serivce2 service) that sets the property. No method injection is set up for that with Autofac.

Is there any way that we'd end up somehow with a reference to B.Service2 somewhere that would stop the context from being unloaded? I don't think so as I basically think this through out loud, but there's just something in the back of my mind itching at me about "what happens with transitive references that maybe we don't interact with?"

Actually, writing this out, if someone has used method injection, are we holding onto any references to those event handlers by the time the assembly load context is going to be declared safe for unload? I... don't think so, but figured I'd raise it.

[tillig]

I'm curious if this if/elseif thing can be consolidated somehow since it's identical code in the blocks. I mean, I don't care a lot but it seems duplicate-y.

[tillig]

It also looks like this exact if/else shows up in ReflectionCacheTupleDictionary under GetKeyAssemblies. Unclear if there's value in some sort of GetKeyAssemblies internal sort of method that handles the Clear() and PopulateAllReferencedAssemblies manipulation.

[alistairjevans]

Aye, there is some duplication there; I've factored the if is Type check out into a new TypeAssemblyReferenceProvider.PopulateAllReferencedAssemblies method that accepts a MemberInfo.

I've included the "reset" of the pre-allocated hashset in the factored-out method as well, although I will say that while I can't quite put my finger on it, it feels icky somehow to reset a locally-defined set inside another method external to the class; introduces coupling of behaviour maybe? Perhaps I'd already done that by accepting a pre-allocated set. Anyway, see what you think.

[tillig]

I think it's fine. I thought about signaling the modifiability by passing it as ref or something, but it's all internal and the kinda icky "side effect" of clearing the set should probably just be known given the context.

[alistairjevans]

Assuming we have a custom ALC context, with assembly A and assembly B; if we explicitly load A into context, B will be also loaded into context, unless (by default) it is already loaded into the default ALC.

So, when we discard the assembly context, if there is any references to B in our cache because A indirectly added them, they will also be wiped from the cache.

I am however going to add a test to verify that if a module in a loaded assembly registers an OnActivated callback and a LifetimeScopeEnding callback where those callbacks are defined in the loaded assembly, that we can still unload. I don't foresee there being an issue though.

I'll also note that this is obviously best-effort; if someone captures a reference to something in a loaded assembly that we aren't tracking, unload likely won't work.

[alistairjevans]

OK, added those extra tests for method injection, no issues. Fundamentally those method injections are actually added as custom middleware to the registration, so they will all get discarded when the component registry goes away.

[tillig]

⛵

[tillig]

Looks great. I didn't merge it in case you had other stuff you wanted to throw in, but I think it's fine as-is.

[alistairjevans]

Thanks, merged. I will need to add docs for this when we release.

Is there anything else planned that would go into the v7 release? Or is this the last item?

[tillig]

I think we caught all the major ones - marked that delegate factory thing obsolete, added this, added the required property support... I haven't searched the issues yet for any mention of "major" but I think this was it.