0

I'm trying to make a N-layer architecture for my Telegram Bot. I created DAL, BLL and PL. I would like to add entity News to my DB. But I have some issue with my context.

My DB Context:

public class ApplicationContext : DbContext
    {
        public DbSet<News> News { get; set; }
        public DbSet<User> Users { get; set; }

        public ApplicationContext(DbContextOptions<ApplicationContext> options) : base(options)
        {           
        }
               
        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<News>().Property(tn => tn.Id).ValueGeneratedOnAdd();
            modelBuilder.Entity<User>().Property(tn => tn.Id).ValueGeneratedOnAdd();

            modelBuilder.Entity<News>().Property(tn => tn.Title).IsRequired();
            modelBuilder.Entity<News>().Property(tn => tn.Href).IsRequired();
            modelBuilder.Entity<News>().Property(tn => tn.Image).IsRequired();
            modelBuilder.Entity<News>().Property(tn => tn.Date).IsRequired();

            modelBuilder.Entity<User>().Property(tn => tn.UserId).IsRequired();
            modelBuilder.Entity<User>().Property(tn => tn.UserName).IsRequired();
            modelBuilder.Entity<User>().Property(tn => tn.DateOfStartSubscription).IsRequired();
            base.OnModelCreating(modelBuilder);
        }
    }

Interface UoW:

public interface IUnitOfWork : IDisposable
    {
        INewsRepository News { get; }
        IUserRepository Users { get; }
        int Complete();
    }

Class UoW:

 public class UnitOfWork : IUnitOfWork
    {       
        public IUserRepository Users { get; }
        public INewsRepository News { get; }
        private readonly ApplicationContext _context;

        public UnitOfWork(ApplicationContext context)
        {
            _context = context;
            Users = new UserRepository.UserRepository(_context);
            News = new NewsRepository.NewsRepository(_context);
        }

        public int Complete() =>  _context.SaveChanges();

        public void Dispose() => _context.Dispose();
    }

My DAL Generic Repository:

async Task IGenericRepository<T>.AddAsync(T entity) => await _context.Set<T>().AddAsync(entity);

DAL Injection:

 public static class DALInjection
    {
        public static void Injection(IServiceCollection services)
        {
            services.AddTransient(typeof(IGenericRepository<>), typeof(GenericRepository<>));
            services.AddTransient<IUserRepository, UserRepository.UserRepository>();
            services.AddTransient<INewsRepository, NewsRepository.NewsRepository>();
            services.AddTransient<IUnitOfWork, UnitOfWork.UnitOfWork>();
        }
    }

My BLL Service class:

 public class ParserService : IParser
    {
     private IUnitOfWork _unitOfWork;
     private readonly IMapper _mapper;

    public ParserService(IUnitOfWork unitOfWork, IMapper mapper)
        {
            _unitOfWork = unitOfWork;
            _mapper = mapper;
        }

 private async Task SaveArticles(IEnumerable<NewsDTO> articlesDTO)
        {            
            var articles = _mapper.Map<IEnumerable<NewsDTO>, IEnumerable<News>>(articlesDTO);
            await _unitOfWork.News.AddAsync(articles.First());
            _unitOfWork.Complete();
        }

BLL Injection:

public static class BLLInjection
    {
        public static void Injection(IServiceCollection services)
        {
            DALInjection.Injection(services);
            services.AddTransient<IParser, ParserService>();
            services.AddTransient<IArticleService, ArticleService>();
            services.AddAutoMapper(typeof(CommonMappingProfile));
        }
    }

My PL:

   private static async Task SendArticleAsync(long chatId, int offset, int count)
        {
            var articles = await _parser.MakeHtmlRequest(offset, count);
            foreach (var article in articles)
            {
                var linkButton = KeyboardGoOver("Перейти", article.Href);
                await _client.SendPhotoAsync(chatId: chatId, photo: article.Image,
                        caption: $"*{article.Title}*", parseMode: Telegram.Bot.Types.Enums.ParseMode.Markdown, replyMarkup: linkButton);

            }
            await OnLoadMoreNewsAsync(chatId, offset + count, count);
        }

PL Startup class:

public void ConfigureServices(IServiceCollection services)
        {
            services.AddControllers();
           
            services.AddDbContext<ApplicationContext>(options =>
                options.UseSqlServer(
                    Configuration.GetConnectionString("DefaultConnection"),
                    b => b.MigrationsAssembly(typeof(ApplicationContext).Assembly.FullName)));
            BLLInjection.Injection(services);
           
            services.AddSwaggerGen(c =>
            {
                c.SwaggerDoc("v1", new OpenApiInfo { Title = "TelegramBot.WebApi", Version = "v1" });
            });
        }

When I tried to debug, I had this error but I could not resolve this issue.

_context = Database = {"Cannot access a disposed context instance. A common cause of this error is disposing a context instance that was resolved from dependency injection and then later trying to use the same context instance elsewhere in your application. This may o...

Could someone help me with this issue?

Ilya
  • 5
  • 3
  • 3
    The bug is in the code you didn't post. Don't use the "generic repository" antipattern to begin with. A DbContext is already a Unit-of-Work, a DbSet is already a Repository. ORMs like EF Core already abstract the persistence mechanism. That `AddAsync` doesn't do what you think it does and isn't needed. In this case the missing code somehow tried to use a singleton DbContext instance. – Panagiotis Kanavos Sep 20 '21 at 09:20
  • Start with plain old EF Core code instead of trying to use "best practices". Right now nobody knows what's going on, except to say that the "repository" and "unit of work" classes aren't working. A properly configured DbContest works just fiine. Using `AddDbContext` works fine. If you add your DbContext as a dependency to a controller it will work fine and only persist changes when you call `SaveChanges` once at the very end of an action. If you don't all changes are discarded. That's U-o-W out-of-the-box – Panagiotis Kanavos Sep 20 '21 at 09:23
  • 1
    Could you explain why "generic repository" is antipattern? If I will have a lot of repositories, it'll be useful for don't repite my code. Which code could I post which could help you to understand my issue? – Ilya Sep 20 '21 at 09:40
  • Why would you need even *one* repository? The DbSet is the repository. The DbContesxt is the unit of work. Why write that `AddAsync` method at all? As for the code you need to post, it's the code that stores the `DbContext` instance to `_context`. Clearly, you're using a Singleton instance when you shouldn't. You'll probably need to post the full UoW and action code, because whatever that code does, isn't standard or common – Panagiotis Kanavos Sep 20 '21 at 09:45
  • As for what's wrong in general [No need for repositories and unit of work with Entity Framework Core](https://gunnarpeipman.com/ef-core-repository-unit-of-work/) explains it in detail. – Panagiotis Kanavos Sep 20 '21 at 09:45
  • I read a lot of topic about useful pattern Repository and UoW and there are different opinion. I used this pattern just for learning on my pet-project. Thanks for the link. – Ilya Sep 20 '21 at 09:57
  • @PanagiotisKanavos Just mentioning a single article does not make it an anti-pattern EF Core mixes/combines several concerns: persistence, UoW, repository. From the perspective of Clean Architecture / Onion Architecture / DDD, I maybe don't want my business logic to reference some DB stuff like EF Core just to access the repository. If Business logic should be persistent-agnostic, so no need to save. In those cases, a generic repository is definitely a valid option and not an anti-pattern. – mu88 Sep 20 '21 at 09:58
  • I edited my question and added UoW interface and class. Please, help me if it's possible. – Ilya Sep 20 '21 at 10:05
  • @mu88 no, the serious, well-known problems it introduces make it an antipattern. There are several dozens of articles that explain why *in this case* it's an anti-pattern. Besides, if you call EF Core "some DB stuff' what is your own "generic repository"? They are both persistence agnostic. EF Core connects to *multiple* providers. Not just relational databases either. As long as there's a provider for a data store, EF Core will work – Panagiotis Kanavos Sep 20 '21 at 10:12
  • @mu88 the very fact questions identical to this pop up almost twice a week is a very strong indication that something is wrong with the "generic repository" (which usually is just a DAO). Not the repository pattern itself (EF Core's DbSet is a Repository after all) but trying to put a low-level "generic" repo on top of a high-level UoW+multi-Repo abstractions – Panagiotis Kanavos Sep 20 '21 at 10:13
  • @Ilya, try to remove `_context.Dispose()`. Context should be disposed by DI. Anyway I'm also hates this pattern. Usually it's do nothing but increases boilerplate in the code. – Svyatoslav Danyliv Sep 20 '21 at 10:16
  • @Ilya where is `UnitOfWork` created? Where does its `context` come from? What is that BLL (not the acronyms, the actual classes) and how is it instantiated? What does your actual controller action look like? Somehow, instead of using the controller's injected DbContext you end up using an old instance – Panagiotis Kanavos Sep 20 '21 at 10:16
  • @Ilya are you using `async void` in your code? That signature is *only* meant for event handlers. Such methods can't be awaited, so if you call such a method from your controller, the controller won't await it, the request will terminate and any scoped objects, including the DbContext, will be disposed – Panagiotis Kanavos Sep 20 '21 at 10:18
  • @PanagiotisKanavos I added more information about my bot. May be it could help – Ilya Sep 20 '21 at 10:33
  • Not yet, but it does demonstrate just how overcomplicated the project is already. *Where's the controller*? That's what handles the request and defines the service scope. Post enough code so people can reproduce the problem – Panagiotis Kanavos Sep 20 '21 at 10:37
  • PL layer is controller. Method "SendArticleAsync" is method of contoller. When I push button on Telegram, this method will be called. I could share my GitHub link, if it's help. – Ilya Sep 20 '21 at 10:41
  • I believe that your problem is `public void Dispose() => _context.Dispose();` in `UnitOfWork` class. `_context` will be disposed twice: once when IoC disposes `UnitOfWork` and once again when IoC disposes `ApplicationContext`. – Artur Sep 20 '21 at 10:53
  • Never dispose instances that you injected via constructor. You can never know what the lifetime of this dependency and who else uses it. Only dispose instances that where created by you. – Artur Sep 20 '21 at 10:56
  • And also `IUnitOfWork` should be registered as scoped and not transient. – Artur Sep 20 '21 at 10:58
  • @Artur I deleted IDisposable and change AddTransient to AddScoped, but it didn't resolve my issue:( – Ilya Sep 20 '21 at 11:30
  • What about `GenericRepository` implementation? Does it disposes context as well? Can you provide the link to your repo? – Artur Sep 20 '21 at 11:37
  • @Artur https://github.com/KlishevichIlya/TelegramBot – Ilya Sep 20 '21 at 13:25

1 Answers1

0

There are few problems in your code.

  1. Controllers are scoped entities, their instances created per http request and disposed after request is finished. It means controller is not good place to subscribe to events. When you call /start endpoint you create an instance of TelegramController and TelegramBotClient, but once the request is finished, the controller and all its non-singleton dependencies (IParser in your case) are disposed. But you subscribed for TelegramBotClient events that captured reference to IParser. It means all events that will arrive after request is finished will try to access disposed IParser instance and this is the reason for your exception.
    For event based messages it's better to use IHostedService. You will need to use IServiceScopeFactory to create a scope for each message and resolve your dependencies from this scope.
public class TelegramHostedService : IHostedService
{
    private IServiceScopeFactory _scopeFactory;

    public TimedHostedService(IServiceScopeFactory scopeFactory)
    {
        _scopeFactory = scopeFactory;
    }

    public Task StartAsync(CancellationToken stoppingToken)
    {
        _client = new TelegramBotClient(_token);
        _client.OnMessage += OnMessageHandlerAsync;
        _client.OnCallbackQuery += OnLoadCallBackAsync;
        _client.StartReceiving(); 

        return Task.CompletedTask;
    }

    public Task StopAsync(CancellationToken stoppingToken)
    {
        // TODO: Unsubscribe from events
        return Task.CompletedTask;
    }

    public static async void OnMessageHandlerAsync(object sender, MessageEventArgs e)
    {
        using var scope = _scopeFactory.CreateScope();
        var handler = scope.ServiceProvider.GetRequiredService<MessageHandler>();
        await handler.Handle(TODO: pass required args); // Move the logic to separate handler class to keep hosted service clean
    }

    ...
}

I moved call to _client.StartReceiving(); after event subscription otherwise there is a chance for race condition when you receive event but you don't yet have subscribers and this event will be lost.

  1. The second issue is as @PanagiotisKanavos said: async void can't be awaited, hence once your code hit first true async method (like DB access, http request, file read or any other I/O operation) the control is returned to the point where async void method was called and continues execution without waiting for operation completion. The whole app can even crash if you throw unhandled exception from such method, hence async void should be avoided. To prevent these problems wrap your async event handlers with sync methods that will block the execution with Wait() method:
public class TelegramHostedService : IHostedService
{
    private IServiceScopeFactory _scopeFactory;

    public TimedHostedService(IServiceScopeFactory scopeFactory)
    {
        _scopeFactory = scopeFactory;
    }

    public Task StartAsync(CancellationToken stoppingToken)
    {
        _client = new TelegramBotClient(_token);
        _client.OnMessage += OnMessageHandler;
        _client.OnCallbackQuery += OnLoadCallBack;
        _client.StartReceiving(); 

        return Task.CompletedTask;
    }

    public Task StopAsync(CancellationToken stoppingToken)
    {
        // TODO: Unsubscribe from events
        return Task.CompletedTask;
    }

    public static void OnMessageHandler(object sender, MessageEventArgs e)
    {
        OnMessageHandlerAsync(sender, e).Wait();
    }

    public static async Task OnMessageHandlerAsync(object sender, MessageEventArgs e)
    {
        using var scope = _scopeFactory.CreateScope();
        var handler = scope.ServiceProvider.GetRequiredService<MessageHandler>();
        await handler.Handle(TODO: pass required args); // Move the logic to separate handler class to keep hosted service clean
    }

    ...
}
Artur
  • 4,595
  • 25
  • 38