Crash on F5

[alexey-gusarov]

Hello!

After fixing issue #38 or #39 I run example ASP.Net.Core.Unity.Example. It's started! But if I trying reload page I got exception:

An unhandled exception occurred while processing the request. InvalidOperationException: No service for type 'Microsoft.AspNetCore.Routing.IEndpointAddressScheme`1[Microsoft.AspNetCore.Routing.RouteValuesAddress]' has been registered. Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)

Stack Query Cookies Headers InvalidOperationException: No service for type 'Microsoft.AspNetCore.Routing.IEndpointAddressScheme`1[Microsoft.AspNetCore.Routing.RouteValuesAddress]' has been registered. Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType) Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider) Microsoft.AspNetCore.Routing.DefaultLinkGenerator.GetEndpoints(TAddress address) Microsoft.AspNetCore.Routing.DefaultLinkGenerator.GetPathByAddress(HttpContext httpContext, TAddress address, RouteValueDictionary values, RouteValueDictionary ambientValues, Nullable pathBase, FragmentString fragment, LinkOptions options) Microsoft.AspNetCore.Routing.LinkGeneratorRouteValuesAddressExtensions.GetPathByRouteValues(LinkGenerator generator, HttpContext httpContext, string routeName, object values, Nullable pathBase, FragmentString fragment, LinkOptions options) Microsoft.AspNetCore.Mvc.Routing.EndpointRoutingUrlHelper.Action(UrlActionContext urlActionContext) Microsoft.AspNetCore.Mvc.UrlHelperExtensions.Action(IUrlHelper helper, string action, string controller, object values, string protocol, string host, string fragment) Microsoft.AspNetCore.Mvc.ViewFeatures.DefaultHtmlGenerator.GenerateActionLink(ViewContext viewContext, string linkText, string actionName, string controllerName, string protocol, string hostname, string fragment, object routeValues, object htmlAttributes) Microsoft.AspNetCore.Mvc.TagHelpers.AnchorTagHelper.Process(TagHelperContext context, TagHelperOutput output) Microsoft.AspNetCore.Razor.TagHelpers.TagHelper.ProcessAsync(TagHelperContext context, TagHelperOutput output) Microsoft.AspNetCore.Razor.Runtime.TagHelpers.TagHelperRunner.RunAsync(TagHelperExecutionContext executionContext) AspNetCore.Views_SharedLayout.b44_1() Microsoft.AspNetCore.Razor.Runtime.TagHelpers.TagHelperExecutionContext.SetOutputContentAsync() AspNetCore.Views_Shared__Layout.ExecuteAsync() Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageCoreAsync(IRazorPage page, ViewContext context) Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageAsync(IRazorPage page, ViewContext context, bool invokeViewStarts) Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderLayoutAsync(ViewContext context, ViewBufferTextWriter bodyWriter) Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderAsync(ViewContext context) Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ViewContext viewContext, string contentType, Nullable statusCode) Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ActionContext actionContext, IView view, ViewDataDictionary viewData, ITempDataDictionary tempData, string contentType, Nullable statusCode) Microsoft.AspNetCore.Mvc.ViewFeatures.ViewResultExecutor.ExecuteAsync(ActionContext context, ViewResult result) Microsoft.AspNetCore.Mvc.ViewResult.ExecuteResultAsync(ActionContext context) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeResultAsync(IActionResult result) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResultFilterAsync<TFilter, TFilterAsync>() Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResultExecutedContext context) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.ResultNext<TFilter, TFilterAsync>(ref State next, ref Scope scope, ref object state, ref bool isCompleted) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeResultFilters() Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter() Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted) Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync() Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync() Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext) Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext httpContext) Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context) Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

ServiceProvider trying to call GetService on already Disposed instance with _container = null.

My loaded assemblies:

github\examples\src\web\ASP.Net.Unity.Example\bin\Debug\netcoreapp2.2\ASP.Net.Core.Unity.Example.dll packages\unity.container\5.10.0\lib\netcoreapp2.0\Unity.Container.dll packages\unity.abstractions\4.1.1\lib\netcoreapp2.0\Unity.Abstractions.dll packages\unity.microsoft.dependencyinjection\5.10.0\lib\netcoreapp1.1\Unity.Microsoft.DependencyInjection.dll github\examples\src\web\ASP.Net.Unity.Example\bin\Debug\netcoreapp2.2\ASP.Net.Core.Unity.Example.Views.dll

[ENikS]

It appears that Microsoft code releases the Service Provider and later call it during page reload. I've filed an Issue with Asp Net project and will wait for their recommendations.

@davidfowl I am assuming it is not a new requirement to still be able to resolve dependencies on the disposed Service Provider?

[davidfowl]

@ENikS no we shouldn;'t be calling GetService on a disposed service provider.

[Hieronymus1]

I see the temporary workaround was rolled back on this commit.

I tried to keep a reference of the IServiceProvider to prevent GC but this unfortunately didn't work because the container is set to null in the Dispose implementation so Unity.Microsoft.DependencyInjection.ServiceProvider.GetService throws an ObjectDisposedException that wouldn't occur if the container wasn't forced to null.

Do you know any workaround to avoid this exception that is systematically thrown after reloading a page?

Could I make a pull request to remove the null assignment that shouldn't be necessarily in the Dispose implementation?

[Hieronymus1]

Removing the null assignation on the container in the ServiceProvider Dispose method fixed the ObjectDisposedException as I did on this branch.

I'm not sure why IDisposable must be implemented in ServiceProvider but it shouldn't be responsible to dispose the container that is not created by the ServiceProvider. IDisposable implementation should only disposed instances they created because they are not responsible to manage the lifetime of external instances.

In any case, removing the null assignation did the trick because the GC won't collect objects that are still referenced int he dependency tree.

Should I make a pull request?

[ENikS]

@Jeromino No, your rationalization is incorrect. Disposed container should be disposed and collected by GC. Keeping a reference alive is undesirable behavior.

The workaround was rolled back so @davidfowl can debug with correct exceptions in place. Until this issue is fixed you could still use the version with the workaround.

[Hieronymus1]

I am sorry but the relation from the ServiceProvider to the IUnityContainer is an aggregation, not a composition. When implementing IDisposable we shouldn't dispose objects that weren't created internally because we are not responsible of their lifetime.

In this case, when using the IServiceProvider.BuildServiceProvider extension with the container instance, as I consumer of the API, I don't expect another object to dispose the instance I created.

When a reference is still kept in the object tree the GC is smart enough to leave it alive but forcing it to null to throw an ObjectDisposedException is only making things worst by breaking the normal behavior.

Could you please confirm in which version was released the temporary workaround?

[davidvedvick]

I'm not so sure this issue will be fixed in .Net core 2.2... judging from this comment on this seemingly related issue: https://github.com/aspnet/AspNetCore/issues/4148#issuecomment-456709196

Would it be worth adding in the workaround for .Net core 2.2?

[ENikS]

I am not sure what could be done here and open to suggestions

[Hieronymus1]

We should not dispose objects we didn't create because we are not responsible of their lifetime. If the API client provides an instance of IUnityContainer it's the external client responsibility to dispose it because our implementation should stay agnostic of the external world and it can't decide the client doesn't need the instance that was provided anymore.

When you create the instance internally, I would suggest to remove the null assignation to prevent garbage collection. This way if your clients are still holding a reference they need on the ServiceProvider the GC will behave as expected.

[ENikS]

We should not dispose objects we didn't create because we are not responsible of their lifetime

Not sure what objects you talking about. The ServiceProvider is explicitly disposed by the framework. It is disposing all the objects it references.

With null assignment it fails with a correct exception, otherwise it would fail with ResolutionFailedException, which is wrong. Disposed container is uncapable of resolving anyting.

The current behavior is all according to spec and if changed would fail compatibility tests.

[Hieronymus1]

I was talking about the IUnityContainer instance passed as an argument to the IServiceCollection.BuildServiceProvider extension and the same applies for the external IServiceCollection passed to ServiceProvider.ConfigureServices.

I looked at the Dispose implementation of UnityContainer and I see we would indeed get ResolutionFailedExceptions for instances kept by the ContainerLifetimeManager. The workaround I first did (to remove the null assignation) only worked on an app where we are only using the TransientLifetimeManager but I've got the ObjectDisposedException again on another app where usage of the ContainerControlledLifetimeManager is required.

The null assignation is actually irrelevant because this is not the problem. It looks like the implementation of IServiceScopeFactory (and thus IDisposable) in the ServiceProvider is not at the right level because there is nothing to dispose in the service provider.

I will temporary repackage with the same workaround you did (not disposing anything) and I will keep you updated if I can find out how to implement IServiceScopeFactory at another level.

[Hieronymus1]

I looked into this again and only the child containers created internally by IServiceScopeFactory.CreateScope should be disposed.

@ENikS would you like to do this fix or you would rather like to receive a pull request?

[ENikS]

I am out of town at the moment and can’t do much. If you can send a pr it would be nice.

[madoxdev]

Sorry, what I don't get really is:

How come the container object is set to NULL when that happens only in the Dispose method of ServiceProvider, but the whole Service Provider seems to maintain the instance (and container set to null)?

Is Service Provider is not Disposed, then how the container is started to be NULL? If the Service Provider is Disposed, how it is accessed to reach a point where the container is null?

[Hieronymus1]

The ServiceProvider is set to null by the framework because of the IServiceScopeFactory that should be implemented at another level. It seems like this is called after each response in ASP.NET so the IServiceScopeFactory should probably rather return a child container and never dispose the root IUnityContainer.

[madoxdev]

The Service Provider is not set to NULL. I've changed the dispose to sth like that: ` #region Disposable

    private void Dispose(bool disposing)
    {
        if (disposing)
        {
            _container?.Dispose();
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    ~ServiceProvider()
    {
        Dispose(false);
    }

    #endregion`

It stopped failing after that.

I tried to not Dispose the _container at all, but then was a huge memory leak.

Also, I tried to Dispose only the Child Container (that is generated inside Service Provider) - also memory leak.

Also, if you take a look here:

 public object GetService(Type serviceType)
        {
            if (null == _container)
            {
                throw new ObjectDisposedException(nameof(IServiceProvider));
            }

            try
            {
                return _container.Resolve(serviceType);
            }
            catch  { /* Ignore */}

            return null;
        }

This is where the error comes from, and it is inside the ServiceProvider (It couldn't be disposed to reach that code).

[Hieronymus1]

The finalizer is redundant because you don't need to dispose native resources though.

I first tried to fix the problem with a standard implementation of IDisposable but this didn't work for me in ASP.NET Core.

I didn't have time to test it but as I said before I had the impression the IServiceScopeFactory is not implemented at the correct level because it shouldn't have the same lifetime as the ServiceProvider.

Also, the ServiceProvider should not be responsible to dispose the container that can also be created externally by the API clients as I use it in my projects. This is an aggregation and it should not dispose objects created outside because it doesn't own their lifetime.

[madoxdev]

I agree with the Dispose of the container. Feels like it shouldn't dispose it. It should dispose only Children that it creates. That sounds like a bigger change though.

That fixes the problem with the minimal amount of changes needed. It worked for me at least.

[Hieronymus1]

There you go. It would be awesome if you can fix this. It's on my list but I am still too swamped at work.

The fix you did didn't work for us though. I first tried to do the exact same thing, with a proper implementation of IDisposable without the null assignation, and it still threw an exception when I tested the integration in our solutions.

For now I rolled back to these packages that are stable:

Unity.Abstractions 4.1.3 Unity.Container 5.10.3 Unity.Interception 5.10.1 Unity.RegistrationByConvention 5.10.0

[madoxdev]

Set of my libraries: .NET Core 2.2 Unity 5.11.1 Unity.Abstractions 5.11.1 Unity.Container 5.11.1 Unity.RegistrationByConvention 5.11.1

When you have a few minutes, please feel free to form from my repo, code is there with a few other smaller changes. I would like to hear back if that helped.

[Hieronymus1]

Why did you close your pull request though? I looked on master and I didn't see the merge revision.

It is because you found out something wasn't working?

[madoxdev]

Its because I strongly agree with not disposing of the Container inside that class. I can re-add that pull request if you wish?

[ENikS]

Not disposing container breaks the MS guidelines and validation checks.

If you want workaround, it is already deployed. No need to re-add it.

[Hieronymus1]

@madoxdev OK I see. The root container should indeed not be disposed by ServiceProvider because the instance can be created outside.

@ENikS you need to read this section on aggregation that explains guidelines on the lifetime of aggregated associations. In short if I give an instance to your API it is not your job to dispose it because you don't know who else may still have a reference that is still required.

I still didn't get to test it but I think we should only dispose child containers created by the IServiceProviderFactory after implementing IServiceProviderFactory outside of the ServiceProvider.

IServiceProvider normally doesn't implement IDisposable.

[ENikS]

Well, problem is with the container. It doesn't have finalizer, so if not disposed it will fail to dispose held instances of container controlled objects.

This needs to be rewritten on more fundamental level.

[Hieronymus1]

The container shouldn't need to implement a finalizer because it doesn't directly hold any native references. I guess we need to implement IDisposable for IServiceProviderFactory and dispose only the child container instance it creates.

Otherwise I think the root container needs to stay alive for the lifetime of the application because the bootstrap is only done once even in a Web host.

[ENikS]

You could try, but it is the service provider being disposed, not the factory.

Framework call dispose on the instance and later tries to resolve something on that same instance.

[Hieronymus1]

Yes this is why I think IServiceProviderFactory should not be implemented at the same level as IServiceProvider. It could probably help to see how this what implemented for other DI frameworks.

[madoxdev]

Agreed, as only one of them inherits from IDisposable, with current code structure whole provider is getting disposed. Not to mention the exception is not telling the truth, and it should be NullReferenceException rather than IServiceProvider disposed.

[davidfowl]

I'm back from vacation and will take a look at the repro? Was any progress made?

[ENikS]

Nothing has changed

[davidfowl]

Here's the reason for the bug https://github.com/aspnet/Extensions/issues/1301#issuecomment-524932471

[ENikS]

Fixed in 5.11.0