jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
11.17k stars 1.18k forks source link

Hosted Service w/MediatR memory issue(s) #488

Closed stevestokes closed 4 years ago

stevestokes commented 4 years ago

So I've been working on this for about a week or so and it's driving me mad. I have a hosted service which runs background services that check a database. Those calls are through MediatR and then into ef core. I noticed my container would flip on aws once in a while, so reduced the timing of my service calls to the db (1ms), and yep, running out of memory. I started to debug if the issue was Lamar (my IoC container) and it appears it is not. I've tried all the configurations I can possibly mess with and I'm having no luck. My latest path has lead me to believe it's an issue with MediatR and how it spins up DbContext's.

For context here's my application's flow: Startup->HostedService.StartAsync()->ServiceWorker()->Run 2 Services-> -->Service1.ExecuteAsync()->MediatR.Send()->Handler->db.Query().ToListAsync() -->Service2.ExecuteAsync()->MediatR.Send()->Handler->db.Query().ToListAsync()

Libraries I'm using .Net Core 3.1 EF Core 2.21 (tried, 3.x, saw same issues) MediatR 8.0.0 MediatR.Microsoft.DependencyInjection (8.0.0) Lamar (4.1.0) (used to be StructureMap) Lamar.Microsoft.DependencyInjection (4.1.0) Contoso test script/database

Startup

        public static async Task Main(string[] args)
        {
            try
            {
                var host = new HostBuilder()
                    .UseLamar<LamarRegistry>()
                    .ConfigureHostConfiguration(configHost =>
                    {
                        configHost.SetBasePath(Directory.GetCurrentDirectory());
                        configHost.AddJsonFile("hostsettings.json", optional: true);
                        configHost.AddEnvironmentVariables();
                        configHost.AddCommandLine(args);
                    })
                    .ConfigureAppConfiguration((hostContext, configApp) =>
                    {
                        configApp.AddEnvironmentVariables();
                        configApp.AddCommandLine(args);
                        configApp.AddUserSecrets<Program>(false);
                    })
                    .ConfigureServices((hostContext, services) =>
                    {
                        var conn = hostContext.Configuration["DB_CONN"];
                        var dbPassword = hostContext.Configuration["DB_PASSWORD"];

                        var builder = new SqlConnectionStringBuilder(conn)
                        {
                            Password = dbPassword
                        };

                        services.AddDbContext<ContosoContext>(ctx => ctx.UseSqlServer(builder.ConnectionString), ServiceLifetime.Transient);

                        // MediatR
                        services.AddMediatR(cfg => cfg.AsTransient(), Assembly.GetExecutingAssembly());
                        services.AddHostedService<ServiceWorker>();
                    })
                    .UseConsoleLifetime()
                    .Build();

                await host.RunAsync();
            }
            catch (Exception ex)
            {
                Console.WriteLine($"CRITICAL - Task Main():{ex.ToString()}");
            }
        }

Registry

    public class LamarRegistry : ServiceRegistry
    {
        public LamarRegistry()
        {
            Scan(_ =>
            {
                _.WithDefaultConventions();
                _.TheCallingAssembly();
                _.AssemblyContainingType<ContosoContext>();

                // MediatR
                _.ConnectImplementationsToTypesClosing(typeof(IRequestHandler<,>));
                _.ConnectImplementationsToTypesClosing(typeof(IRequestHandler<>));
                _.ConnectImplementationsToTypesClosing(typeof(INotificationHandler<>));
            });

            For<ContosoContext>().Use<ContosoContext>().Transient();

            // MediatR
            For<IMediator>().Use<Mediator>().Transient();

            Policies.SetAllProperties(y => y.OfType<ContosoContext>());
            Policies.SetAllProperties(y => y.OfType<IHostEnvironment>());
            Policies.SetAllProperties(y => y.OfType<IConfiguration>());
            Policies.SetAllProperties(y => y.OfType<IContainer>());
            Policies.SetAllProperties(y => y.OfType<IServiceProvider>());

            // MediatR
            Policies.SetAllProperties(y => y.OfType<IMediator>());
        }
    }

ServiceWorker

        public Task StartAsync(CancellationToken cancellationToken)
        {
            var svc1 = _container.GetInstance<Service1>();
            var svc2 = _container.GetInstance<Service2>();

            Task.Run(async () => await svc1.ExecuteAsync(cancellationToken));
            Task.Run(async () => await svc2.ExecuteAsync(cancellationToken));

            return Task.CompletedTask;
        }

Service1 & 2 are similar, they just call different Message/Handlers

    public class Service1 : BaseJob, IAsyncJob
    {
        public async Task ExecuteAsync(CancellationToken cancellationToken)
        {
            while (!cancellationToken.IsCancellationRequested)
            {
                var items = await _mediator.Send(new GetCourses());

                Console.WriteLine($"Hello from {GetType().Name}, ItemCount: {items.Count()}");

                await Task.Delay(TimeSpan.FromMilliseconds(1));
            }
        }
    }

Messages/Handlers - I try using IContainer (scoped/transient) and setter injected IContainer setter injected DbContext as well as injection via constructor. Below is how I normally use it with GetCourses - setter injection in base class with DbContext

    public class Handler
    {
        public class GetByItemsHandler : HandlerBase, IRequestHandler<GetCourses, IEnumerable<Course>>
        {
            public async Task<IEnumerable<Course>> Handle(GetCourses message, CancellationToken cancellationToken)
            {
                return await _db.Courses.ToListAsync();
            }
        }

        public class GetStudentGradesHandler : HandlerBase, IRequestHandler<GetStudentGrades, IEnumerable<StudentGrade>>
        {
            public async Task<IEnumerable<StudentGrade>> Handle(GetStudentGrades message, CancellationToken cancellationToken)
            {
                //return await _db.StudentGrades.ToListAsync();

                using (var scope = _services.CreateScope())
                {
                    var db = scope.ServiceProvider.GetRequiredService<ContosoContext>();

                    return await db.StudentGrades.ToListAsync();
                }
            }
        }
    }

    public class GetCourses : IRequest<IEnumerable<Course>>
    {
        public GetCourses()
        {
        }
    }

    public class GetStudentGrades : IRequest<IEnumerable<StudentGrade>>
    {
        public GetStudentGrades()
        {
        }
    }

HandlerBase, all are set by setter injection

    public abstract class HandlerBase
    {
        public IServiceProvider _services { get; set; }
        public IContainer _container { get; set; }
        public ContosoContext _db { get; set; }
        public IMediator _mediator { get; set; }
    }

Memory on aws (ecs/docker) each peak is a container roll-over: image

Memory in VS2019 image

Memory details image

Example- clone/run this. You will need to add a secrets file, example below

https://github.com/stevestokes/hosted-service-test

{
  "DB_CONN": "Server=your-db;Database=Contoso;User Id=your-db-user",
  "DB_PASSWORD": "your-password"
}

Sooooo, questions. 1) Does my config look ok? I have everything set to transient where I can (dbcontext, mediatr, lamar) 2) Do I need to specify that the dbcontext is transient anywhere else, or invoke it some other way?

I came into this issue when I went from .Net core 2.1 to 3.x. I have a previous service based on that stack running fine, using a similar setup .net core 2.1/ef core 2.x/strcuturemap/mediatr.

lilasquared commented 4 years ago

I'm not familiar with Lamar as a container but is it necessary to register the db context and all mediator objects both in ConfigureServices and in the LamarRegistry class? With structuremap you would only register them once.

You are using what looks like just a single instance of the db context for each service. Since you are not using a nested container of any kind the db context is most likely hanging out for the lifetime of each service. The more you query I would guess the more gets added to the change tracking.

One solution would be to eliminate change tracking, if your services are read only then i would suggest this anyway, though that doesn't resolve the lifecycle problem. I would set up a nested container for each unit of work, in this case you need to construct a nested container inside the while loops. It may be worth while to move those while loops up a level and construct the services themselves from a nested container.

Edit: just noticed the handlers were different so in one you tried using the service scope - i'm guessing that had the same result?

stevestokes commented 4 years ago

Thanks for the reply, some comments in-line:

I'm not familiar with Lamar as a container but is it necessary to register the db context and all mediator objects both in ConfigureServices and in the LamarRegistry class? With structuremap you would only register them once.

Lamar is made by the creator of StructureMap, it's a rewrite. I can see why me doing that in the example may be confusing. Some are necessary, some aren't. I don't think extra registrations are causing this issue, I've played with it quite a bit (commenting them in/out ect). I did modify the ConfigureServices line to just be the connection string, no difference in behavior. services.AddDbContext<ContosoContext>(ctx => ctx.UseSqlServer(builder.ConnectionString))

You are using what looks like just a single instance of the db context for each service. Since you are not using a nested container of any kind the db context is most likely hanging out for the lifetime of each service. The more you query I would guess the more gets added to the change tracking.

I dedicate a single mediatr per Service instance. This then calls Send() messages which spin up new Handler instances with new DbContexts (in the base class). This is where I am not seeing items get disposed. You described what may be the issue, though, I am not sure how to go about correcting it other than the things I've tried or you've described here, none of them seem to work :(

One solution would be to eliminate change tracking, if your services are read only then i would suggest this anyway, though that doesn't resolve the lifecycle problem. I would set up a nested container for each unit of work, in this case you need to construct a nested container inside the while loops. It may be worth while to move those while loops up a level and construct the services themselves from a nested container.

While change tracking may be a culprit here, that's a band-aid not a fix. In practice I do need to make db calls from the hosted service, this is simply just an example I threw together to help diagnose. Note, I saw this issue both with, and without Lamar.

So I did the nested container in the while loop:

        public async Task ExecuteAsync(CancellationToken cancellationToken)
        {
            while (!cancellationToken.IsCancellationRequested)
            {
                using (var scope = _container.CreateScope())
                {
                    var _mediator = scope.ServiceProvider.GetService<IMediator>();

                    var items = await _mediator.Send(new GetCourses());

                    Console.WriteLine($"Hello from {GetType().Name}, ItemCount: {items.Count()}");

                    await Task.Delay(TimeSpan.FromMilliseconds(1));
                }
            }
        }

This did not fix it. Note _container is IServiceScope here.

If I move the while loop up a level into the main IHostedService thread, I am not able to run multiple HostedServices which is why the main start method is built like this:

        public Task StartAsync(CancellationToken cancellationToken)
        {
            var svc1 = _container.GetInstance<Service1>();
            var svc2 = _container.GetInstance<Service2>();

            Task.Run(async () => await svc1.ExecuteAsync(cancellationToken));
            Task.Run(async () => await svc2.ExecuteAsync(cancellationToken));

            return Task.CompletedTask;
        }

For fun, I tried it, same result:

        public IServiceProvider _container { get; set; }

        public async Task StartAsync(CancellationToken cancellationToken)
        {
            while (!cancellationToken.IsCancellationRequested)
            {
                using (var scope = _container.CreateScope())
                {
                    var svc1 = _container.GetRequiredService<Service1>();
                    var svc2 = _container.GetRequiredService<Service2>();

                    await Task.Run(async () => await svc1.ExecuteAsync(cancellationToken));
                    await Task.Run(async () => await svc2.ExecuteAsync(cancellationToken));
                }
            }
        }

Modified Service1

        public async Task ExecuteAsync(CancellationToken cancellationToken)
        {
                using (var scope = _container.CreateScope())
                {
                    var _mediator = scope.ServiceProvider.GetService<IMediator>();

                    var items = await _mediator.Send(new GetCourses());

                    Console.WriteLine($"Hello from {GetType().Name}, ItemCount: {items.Count()}");

                    await Task.Delay(TimeSpan.FromMilliseconds(1));
                }
        }

I did put a code sample up, if you have a moment please clone it and tinker with it. At this point I'm willing to try whatever I can out.

lilasquared commented 4 years ago

I pulled it down and did some tinkering, removing mediator from the equation and just referencing the db contexts directly from the services yields the same result, so its definitely not a problem with MediatR. I know thats not super helpful but it narrows down the search a bit. I also upgraded the project to EFCore 3.1 but that didn't help either.

stevestokes commented 4 years ago

I pulled it down and did some tinkering, removing mediator from the equation and just referencing the db contexts directly from the services yields the same result, so its definitely not a problem with MediatR. I know thats not super helpful but it narrows down the search a bit. I also upgraded the project to EFCore 3.1 but that didn't help either.

I still think it can be a MediatR issue. I modified my example and referenced the DbContext without MediatR - I was not getting the memory issues, this is how I had it:

        public async Task ExecuteAsync(CancellationToken cancellationToken)
        {
            using (var scope = _container.CreateScope())
            {
                var _db = scope.ServiceProvider.GetRequiredService<ContosoContext>();

                var items = await _db.Courses.ToListAsync();

                Console.WriteLine($"Hello from {GetType().Name}, ItemCount: {items.Count()}");

                await Task.Delay(TimeSpan.FromMilliseconds(1));
            }
        }
stevestokes commented 4 years ago

So if I remove Lamar, rely simply on the built in IoC and scope from the Service AND the Handler, I do not get the issue:

Service

        public async Task ExecuteAsync(CancellationToken cancellationToken)
        {
            using (var scope = _container.CreateScope())
            {
                var _mediator = scope.ServiceProvider.GetService<IMediator>();

                var items = await _mediator.Send(new GetCourses());

                Console.WriteLine($"Hello from {GetType().Name}, ItemCount: {items.Count()}");

                await Task.Delay(TimeSpan.FromMilliseconds(1));
            }
        }

Handler

        public class GetByItemsHandler : HandlerBase, IRequestHandler<GetCourses, IEnumerable<Course>>
        {
            public IServiceProvider _container { get; set; }

            public GetByItemsHandler(IServiceProvider container)
            {
                _container = container;
            }

            public async Task<IEnumerable<Course>> Handle(GetCourses message, CancellationToken cancellationToken)
            {
                using (var scope = _container.CreateScope())
                {
                    var db = scope.ServiceProvider.GetRequiredService<ContosoContext>();

                    return await db.Courses.ToListAsync();
                }
            }
        }

Hmm. The issue is this pattern doesn't work for my needs. I have many (500+) handlers and I use Setter Injection everywhere so I don't have to write a bunch of ctor's. I may just revert to .net core 2.1/structuremap and call it day lol

lilasquared commented 4 years ago

Why would you have to make two scopes? And what’s the difference in CreateScope vs GetNestedContainer? If you put Lamar back and double scope is it still an issue? Maybe it’s Lamar doing something weird?

lilasquared commented 4 years ago

@stevestokes i was able to get rid of the memory problem as well but did not require the level of scoping you showed. I moved the scoping up to the service worker itself, though that might not even be necessary. If you comment .UseMicrosoft() in the Program.cs and uncomment .UseLamar(...) then the memory issues come back. Seems like a problem with Lamar, or registration is wrong.

https://github.com/lilasquared/hosted-service-test

lilasquared commented 4 years ago

@stevestokes cross posted this over to the lamar repo, might be related to some other issues they are having with IDisposables. You should be able to use StructureMap with .net core 3.1, its not quite as straightforward as lamar but not too difficult. My fork of your hosted-service-test has an example of it.

stevestokes commented 4 years ago

@lilasquared awesome, thanks. I'll take a look tonight, been swamped lately.

stevestokes commented 4 years ago

@lilasquared - I switched to StructureMap and kept everything the same, no issues. It does look like there is an issue with Lamar and Nested Containers. Closing this one, thanks a bunch!