Memory/EF issues with the way queries are created

[colinangusmackay]

We've been getting an issue where EF Core logs the following: More than twenty 'IServiceProvider' instances have been created for internal use by Entity Framework. This is commonly caused by injection of a new singleton service instance into every DbContext instance. . We also noticed that after running for a short period of time the process was taking 6+Gb of RAM on our server.

We think we tracked it down to the fix for issue #9, i.e. this commit: 2a07b68

Basically, it seems that creating all those service providers is causing EF Core issues.

Our fix works for us, although it may not work in every scenario.

Initially, what we did was in the ServiceCollectionExtensions.AddDarker method we changed the services.AddSingleton<IQueryProcessor> line to:

services.AddSingleton<IQueryProcessor>(sp =>
{
  factory.SetProvider(sp);
  var queryProcessor = builder.Build();
  return queryProcessor;
});

And added a SetProvider() method to the AspNetHandlerFactory class, which we had to extract and rename because it was internal.

public sealed class AcHandlerFactory : IQueryHandlerFactory, IQueryHandlerDecoratorFactory
{
  private IServiceProvider _services;
  public void SetProvider(IServiceProvider serviceProvider)
  {
    if (_services != null)
      throw new InvalidOperationException($"A service provider is already assigned to the {nameof(AcHandlerFactory)}.");
    _services = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));
  }

  IQueryHandler IQueryHandlerFactory.Create(Type handlerType)
  {
    if (_services == null)
      throw new InvalidOperationException($"A service provider must be assigned to the {nameof(AcHandlerFactory)} in advance of its use.");

    var result = (IQueryHandler)_services.GetService(handlerType);
    return result;
  }
// ... other things in the class
}

That fixed the initial issue... But since the service provider passed to the Singleton's new factory function isn't scoped, anything created that is scoped, say, something working inside a web request, causes the DI framework to break when calling GetService() on the IServiceProvider.

So.... The next step was to ensure those didn't break either. So, I now create a new scope for every query (we've done this kind of thing in Brighter before, and works well in back end applications such as windows services where there is no natural scope for a single piece of work)

That changes the handler factory to this:

   public sealed class AcHandlerFactory : IQueryHandlerFactory, IQueryHandlerDecoratorFactory
    {
        private IServiceProvider _services;
        private readonly Dictionary<IQueryHandler, IServiceScope> _scopes = new Dictionary<IQueryHandler, IServiceScope>();

         public AcHandlerFactory()
        {
        }

         public void SetProvider(IServiceProvider serviceProvider)
        {
            if (_services != null)
                throw new InvalidOperationException($"A service provider is already assigned to the {nameof(AcHandlerFactory)}.");
            _services = serviceProvider ?? throw new ArgumentNullException(nameof(serviceProvider));
        }

         IQueryHandler IQueryHandlerFactory.Create(Type handlerType)
        {
            if (_services == null)
                throw new InvalidOperationException($"A service provider must be assigned to the {nameof(AcHandlerFactory)} in advance of its use.");

             var scope = _services.CreateScope();
            var result = (IQueryHandler)scope.ServiceProvider.GetService(handlerType);
            _scopes.Add(result, scope);
            return result;
        }

         void IQueryHandlerFactory.Release(IQueryHandler handler)
        {
            if (_scopes.TryGetValue(handler, out var scope))
            {
                scope.Dispose();
                _scopes.Remove(handler);
            }
        }

         T IQueryHandlerDecoratorFactory.Create<T>(Type decoratorType)
        {
            return (T)_services.GetService(decoratorType);
        }

         void IQueryHandlerDecoratorFactory.Release<T>(T handler)
        {
            // no op
        }
    }

The decorator part is not scoped here, but it may be useful to do that too.

I think the handler factory here needs to be extended to provide for a wider array of use cases (or have a way to pick a factory based on a specific use case). Instantiating a new service provider on each query appears to be hard on memory, at least when coupled with EF Core.

[holytshirt]

Thanks, I'll try have a look on Tuesday next week.

Nice explanation! I think I know what to do for this.

[holytshirt]

I have been working on this. I'm trying hard not to introduce a breaking change.
I don't think I can :(

[holytshirt]

Hi @colinangusmackay this has been a pain. I manage to get a better version of the registration working and just pushed to Nuget. Darker version 2.0.63

I've changed it to work very similar to Brighters registration.

1. No longer instantiating a new service provider each query
2. You can change the lifetime of the queryprocessor to Scoped https://github.com/BrighterCommand/todo-backend-aspnetcore-brighter/blob/master/src/ToDoApi/Startup.cs#L78-L85

I had to rewrite the reg stuff, it was difficult not to break anything. Let me know if this fixes your issue in your code.

Toby

[colinangusmackay]

I should hopefully have the time to try this out on Friday.... I'll let you know the outcome.

[colinangusmackay]

As promised, an update

Checked and confirmed two out of three applications are working well with this fix. The last application is more complex and taking more time... and that's the one that caused us most problems with Darker previously, so I'm treading more carefully with it.

[holytshirt]

Awesome news, let me know how it goes and we'll close the bug once you have confirmed all is good.

We have also brought this into a project at huddle which uses the MS DI and it is working well.

[colinangusmackay]

Okay - Stress testing done. The process memory stayed reasonable. I didn't see the error More than twenty 'IServiceProvider' instances have been created for internal use by Entity Framework. in the logs. We're all happy here. 😃 Many thanks for fixing this issue.

[holytshirt]

@colinangusmackay I missed registering the default decorator for the fallback policy.
I've fixed it now with release 2.0.78. I also tweaked the registration code and have cleaned it up a lot. Always easier to look back at code a short period after writing it.