I have implemented service which working with db context. I am using entity framework and I do not need repositories because logic is quite simple.

I want to keep simple logic but clean approach. One problem I have is when I want to call service method 2 times in same service instance. I will get context dispose exception. this is not problem in my case because for now I do not have case like that. But maybe you have idea for better solution.

my code:

using Common.Configurations;
using Common.Container;
using Common.Factory;
using Common.Helpers;
using Common.Services;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using RemoteManager.EntityFramework;
using Common.Entities;

namespace RemoteManagerApi.Services
{
    public class MachineService : IMachineService
    {
        private readonly IOptions<ApplicationConfiguration> applicationConfiguration;
        private readonly ILogger<MachineService> logger;
        private readonly IMachineCleaner machineCleaner;
        private readonly IFileContainer fileContainer;
        private readonly RemoteManagerContext context;

        public MachineService(RemoteManagerContext context, IFileContainerFactory fileContainerFactory, IOptions<ApplicationConfiguration> applicationConfiguration, IMachineCleaner machineCleaner, ILogger<MachineService> logger)
        {
            this.fileContainer = fileContainerFactory.CreateContainer();
            this.applicationConfiguration = applicationConfiguration;
            this.logger = logger;
            this.machineCleaner = machineCleaner;
            this.context = context;
        }

        public async Task<MachineConfiguration> Add(MachineConfiguration machine)
        {
            try
            {
                await this.fileContainer.CreateDirectory(machine.SessionFolder);
                logger.LogInformation($"Start  {nameof(MachineService)}:{nameof(this.Add)}");
                using (var db = this.context)
                {
                    var res = db.MachineConfigurations.Add(machine);
                    db.SaveChanges();
                    return await Task.FromResult(res);
                }
            }
            catch
            {
                logger.LogError($"Error  {nameof(MachineService)}:{nameof(this.Add)}");
                throw;
            }
        }

        public async Task<bool> Delete(int id)
        {
            try
            {
                logger.LogInformation($"Start  {nameof(MachineService)}:{nameof(this.Delete)}");
                using (var db = this.context)
                {
                    var machineConfiguration = db.MachineConfigurations.FirstOrDefault(f => f.Id == id);
                    var isDeleted = db.MachineConfigurations.Remove(machineConfiguration);
                    db.SaveChanges();
                    return await Task.FromResult(true);
                }
            }
            catch
            {
                logger.LogError($"Error  {nameof(MachineService)}:{nameof(this.Delete)}");
                throw;
            }
        }

        public async Task<MachineConfiguration> Get(int id)
        {
            try
            {
                logger.LogInformation($"Start  {nameof(MachineService)}:{nameof(this.Get)}");
                using (var db = this.context)
                {
                    var machineConfiguration = db.MachineConfigurations.FirstOrDefault(f => f.Id == id);
                    return await Task.FromResult(machineConfiguration);
                }
            }
            catch
            {
                logger.LogError($"Error  {nameof(MachineService)}:{nameof(this.Get)}");
                throw;
            }
        }

        public async Task<IEnumerable<MachineConfiguration>> GetAll()
        {
            try
            {
                logger.LogInformation($"Start  {nameof(MachineService)}:{nameof(this.GetAll)}");
                using (var db = this.context)
                {
                    var machineConfigurations = db.MachineConfigurations.ToList();
                    return await Task.FromResult(machineConfigurations);
                }
            }
            catch
            {
                logger.LogError($"Error  {nameof(MachineService)}:{nameof(this.GetAll)}");
                throw;
            }
        }
    }
}
share|improve this question
    
use a single unit of work class to read and update database(unit of work design pattern). – Muhammad Nasir Nov 10 '16 at 14:40
    
Unit of work goes together with the Repository pattern. All that is unnecessarily complicated in my case. – Raskolnikov Nov 10 '16 at 14:48
5  
An EF DbContext is a unit of work - it encapsulates a transaction. Wrapping it with a full-blown UoW+Repository pattern isn't just overkill, it's outright wrong IMO. And it wouldn't solve the "who's responsible for disposing the context" problem. – Mat's Mug Nov 10 '16 at 14:50
    
Also... if you look closely at the class' public interface, it definitely looks like a generic repository anyway. – Mat's Mug Nov 10 '16 at 15:08
    
Unit of work and repository pattern can be useful if we want to decouple data access and entity framework. For example if in future we need to replace entity framework with something else. In my case they are strongly coupled. But as I said, I don't need that. – Raskolnikov Nov 10 '16 at 15:19

You're injecting a disposable resource (context) - this means someone else is creating it (which is excellent BTW), and that someone else should therefore be responsible for disposing the context.

But your code is wrapping context accesses in a using block, which is going to call Dispose on that context when execution leaves the using scope.

Remove the using scopes - it's not this class' job to dispose the context. If you're using an IoC container for dependency injection, it's the container that's owning the instances, and therefore it's the container's responsibility to dispose them. If you're not using an IoC container, then whoever is newing up the RemoteManagerContext is the owner that needs to dispose it.

share|improve this answer
    
I am using IoC in Startup: "services.AddTransient<RemoteManagerContext>(_ => new RemoteManagerContext(connectionString));". DbContext is unmenaged resource. Can I be sure that is Disposed ? – Raskolnikov Nov 10 '16 at 15:11
    
DbContext itself is a managed resource, it's just IDisposable. I don't know which DI framework you're using, but with Ninject the transient scope (which is the default) does not handle IDisposable dependencies. With Ninject I'd bind it .InCallScope() or, if this is a web/MVC application, .InRequestScope() so as to get one instance per request. If you really want to be 100% certain that your container is calling Dispose, you could make a thin wrapper for the context, implement IDisposable, and place a breakpoint in your Dispose implementation, or just TRACE-log the disposal. – Mat's Mug Nov 10 '16 at 15:15
    
Microsoft.Extensions.DependencyInjection. I am working on ASP.NET Core project. – Raskolnikov Nov 10 '16 at 15:24
    
Per this page, it seems you need to work with some IServiceScope - again I don't know the specifics, but it seems it's just a matter of properly configuring the IoC container and your resources will be disposed as they should. – Mat's Mug Nov 10 '16 at 15:28
    
Thank you for your help. – Raskolnikov Nov 10 '16 at 15:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.