2

I may know the answer to my posted question: I'm using constructor dependency injection throughout the entire application which is a looped C# console application that does not exit after each request.

I suspect the life time of all of the included objects is essentially infinite due to this. When attempting to adjust the life time while registering, it warns that a transient object cannot be implemented on a singleton object due to dependencies (which inspired looking at memory utilization and this question).

This is my first ground up console application, a bot, that logs into a service provider and waits for messages. I come from .NET Core Web API which again has dependencies all over, but I think the key difference here is below all of my code is the platform itself which handles each request individually then kills the thread that ran.

How close am I? Would I have to separate the bot itself from the base console application listening to the service provider and attempt to replicate the platform that IIS/kestrel/MVC routing provides to separate the individual requests?

Edit: Originally I intended this question as more of a design principal, best practice, or asking for direction direction. Folks requested reproducible code so here we go:

namespace BotLesson
{
    internal class Program
    {
        private static readonly Container Container;

        static Program()
        {
            Container = new Container();
        }

        private static void Main(string[] args)
        {
            var config = new Configuration(args);

            Container.AddConfiguration(args);
            Container.AddLogging(config);

            Container.Register<ITelegramBotClient>(() => new TelegramBotClient(config["TelegramToken"])
            {
                Timeout = TimeSpan.FromSeconds(30)
            });
            Container.Register<IBot, Bot>();
            Container.Register<ISignalHandler, SignalHandler>();

            Container.Register<IEventHandler, EventHandler>();
            Container.Register<IEvent, MessageEvent>();

            Container.Verify();

            Container.GetInstance<IBot>().Process();

            Container?.Dispose();
        }
    }
}

Bot.cs

namespace BotLesson
{
    internal class Bot : IBot
    {
        private readonly ITelegramBotClient _client;
        private readonly ISignalHandler _signalHandler;
        private bool _disposed;

        public Bot(ITelegramBotClient client, IEventHandler handler, ISignalHandler signalHandler)
        {
            _signalHandler = signalHandler;

            _client = client;
            _client.OnCallbackQuery += handler.OnCallbackQuery;
            _client.OnInlineQuery += handler.OnInlineQuery;
            _client.OnInlineResultChosen += handler.OnInlineResultChosen;
            _client.OnMessage += handler.OnMessage;
            _client.OnMessageEdited += handler.OnMessageEdited;
            _client.OnReceiveError += (sender, args) => Log.Error(args.ApiRequestException.Message, args.ApiRequestException);
            _client.OnReceiveGeneralError += (sender, args) => Log.Error(args.Exception.Message, args.Exception);
            _client.OnUpdate += handler.OnUpdate;
        }

        public void Process()
        {
            _signalHandler.Set();
            _client.StartReceiving();

            Log.Information("Application running");

            _signalHandler.Wait();

            Log.Information("Application shutting down");
        }

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

        protected virtual void Dispose(bool disposing)
        {
            if (_disposed) return;
            if (disposing) _client.StopReceiving();
            _disposed = true;
        }
    }
}

EventHandler.cs

namespace BotLesson
{
    internal class EventHandler : IEventHandler
    {
        public void OnCallbackQuery(object? sender, CallbackQueryEventArgs e)
        {
            Log.Debug("CallbackQueryEventArgs: {e}", e);
        }

        public void OnInlineQuery(object? sender, InlineQueryEventArgs e)
        {
            Log.Debug("InlineQueryEventArgs: {e}", e);
        }

        public void OnInlineResultChosen(object? sender, ChosenInlineResultEventArgs e)
        {
            Log.Debug("ChosenInlineResultEventArgs: {e}", e);
        }

        public void OnMessage(object? sender, MessageEventArgs e)
        {
            Log.Debug("MessageEventArgs: {e}", e);
        }

        public void OnMessageEdited(object? sender, MessageEventArgs e)
        {
            Log.Debug("MessageEventArgs: {e}", e);
        }

        public void OnReceiveError(object? sender, ReceiveErrorEventArgs e)
        {
            Log.Error(e.ApiRequestException, e.ApiRequestException.Message);
        }

        public void OnReceiveGeneralError(object? sender, ReceiveGeneralErrorEventArgs e)
        {
            Log.Error(e.Exception, e.Exception.Message);
        }

        public void OnUpdate(object? sender, UpdateEventArgs e)
        {
            Log.Debug("UpdateEventArgs: {e}", e);
        }
    }
}

SignalHandler.cs

This isn't directly related to my problem, but it is holding the application in a waiting pattern while the third party library listens for messages.

namespace BotLesson
{
    internal class SignalHandler : ISignalHandler
    {
        private readonly ManualResetEvent _resetEvent = new ManualResetEvent(false);
        private readonly SetConsoleCtrlHandler? _setConsoleCtrlHandler;

        public SignalHandler()
        {
            if (!NativeLibrary.TryLoad("Kernel32", typeof(Library).Assembly, null, out var kernel)) return;
            if (NativeLibrary.TryGetExport(kernel, "SetConsoleCtrlHandler", out var intPtr))
                _setConsoleCtrlHandler = (SetConsoleCtrlHandler) Marshal.GetDelegateForFunctionPointer(intPtr,
                    typeof(SetConsoleCtrlHandler));
        }

        public void Set()
        {
            if (_setConsoleCtrlHandler == null) Task.Factory.StartNew(UnixSignalHandler);
            else _setConsoleCtrlHandler(WindowsSignalHandler, true);
        }

        public void Wait()
        {
            _resetEvent.WaitOne();
        }

        public void Exit()
        {
            _resetEvent.Set();
        }

        private void UnixSignalHandler()
        {
            UnixSignal[] signals =
            {
                new UnixSignal(Signum.SIGHUP),
                new UnixSignal(Signum.SIGINT),
                new UnixSignal(Signum.SIGQUIT),
                new UnixSignal(Signum.SIGABRT),
                new UnixSignal(Signum.SIGTERM)
            };

            UnixSignal.WaitAny(signals);
            Exit();
        }

        private bool WindowsSignalHandler(WindowsCtrlType signal)
        {
            switch (signal)
            {
                case WindowsCtrlType.CtrlCEvent:
                case WindowsCtrlType.CtrlBreakEvent:
                case WindowsCtrlType.CtrlCloseEvent:
                case WindowsCtrlType.CtrlLogoffEvent:
                case WindowsCtrlType.CtrlShutdownEvent:
                    Exit();
                    break;

                default:
                    throw new ArgumentOutOfRangeException(nameof(signal), signal, null);
            }

            return true;
        }

        private delegate bool SetConsoleCtrlHandler(SetConsoleCtrlEventHandler handlerRoutine, bool add);

        private delegate bool SetConsoleCtrlEventHandler(WindowsCtrlType sig);

        private enum WindowsCtrlType
        {
            CtrlCEvent = 0,
            CtrlBreakEvent = 1,
            CtrlCloseEvent = 2,
            CtrlLogoffEvent = 5,
            CtrlShutdownEvent = 6
        }
    }
}

My original point is based off of some assumptions I am making on SimpleInject--or more specifically the way I am using SimpleInject.

The application stays running, waiting on SignalHandler._resetEvent. Meanwhile messages come in via any of the handlers on Bot.cs constructor.

So my thought/theory is Main launches Bot.Process which has a direct dependency on ITelegramClient and IEventHandler. In my code there isn't a mechanism to let these resources go and I suspect I was assuming the IoC was going to perform magic and release resources.

However, sending messages to the bot continuously increases the number of objects, according to Visual Studio memory usage. This is reflected in actual process memory as well.

Though, while editing this post for approval, I think I may have ultimately been misinterpreting Visual Studio's diagnostic tools. The application's memory utilization seems to hang out at around 36 MB after 15 minutes of run time. Or it's simply increasing so little at a time that it's difficult to see.

Comparing Memory Usage snapshots I took at 1 minute versus 17 minutes, there appears to have been 1 of each of the objects above created. If I am reading this properly, I imagine that proves the IoC is not creating new objects (or they are being disposed before I have a chance to create a snapshot.

mrUlrik
  • 150
  • 11
  • One way to control the lifetime of your objects is by using the service locator pattern. Instead of injecting the dependencies, inject the IServiceProvider, so when you set the types in the container as transient, you can use them and dispose of them immediately. – insane_developer Aug 04 '20 at 03:54
  • Thank you for that! I've been reading about that idea as well and it completely makes sense. This seems like a very easy solution. That being said though, it seems like something that is very commonly frowned upon: accessing the container from outside of the composite root. – mrUlrik Aug 04 '20 at 04:01
  • that is why all supported containers implement that interface, so you are not tying yourself to a particular IoC container. I'm not saying this is the way to go always. I wouldn't do that in an ASP.NET application, for example. – insane_developer Aug 04 '20 at 04:18
  • Thanks for the suggestion! I'll consider this. It would be very easy to implement. – mrUlrik Aug 04 '20 at 04:21
  • 1
    Related: https://stackoverflow.com/questions/4788358/how-to-determine-memory-usage-in-my-net-application – Steven Aug 04 '20 at 05:54
  • 1
    As an answer in the related question notes: "Memory usage in .Net is very difficult to gauge - It will depend on a lot of factors." .NET eagerly consumes memory, and the numbers in the task manager are not realistic. 35 MB of memory is not much, but you will have to understand that a DI Container can consume quite some memory (when the application is large). And as long as the memory footprint doesn't keep infinitely increasing, there's no memory leak. – Steven Aug 04 '20 at 05:59
  • I'm not seeing anything in your code that would constantly increase memory usage unless the Telegram client itself has some kind of memory leak. – ProgrammingLlama Aug 04 '20 at 06:29
  • 2
    `One way to control the lifetime of your objects is by using the service locator pattern.` The service locator pattern is generally something you want to avoid. Everything that @insane_developer mentions in his comments can be done in many IoC containers (e.g. Autofac) without using service locator. – mjwills Aug 04 '20 at 06:57
  • @mjwills I am well aware of what you are mentioning. If you know of a clean way to inject transient dependencies that can be disposed of right after usage in a Console Application, please do share. I do not want all the IDisposable instances to be taken care when the program exits. Think of a class that needs DbContext and has to get a fresh DbContext in different methods. I'm all for a better approach. – insane_developer Aug 04 '20 at 14:43
  • @mjwills by the way, I was using the Microsoft IoC container to do this, adding services to the pipeline using the HostBuilder. It seems that you were implying that my answer did not use a container. You can plug in other containers that implement IServiceProvider. – insane_developer Aug 04 '20 at 14:46
  • `If you know of a clean way to inject transient dependencies that can be disposed of right after usage in a Console Application, please do share.` I mentioned Autofac in my earlier comment. https://autofaccn.readthedocs.io/en/latest/resolve/relationships.html `Owned` or `Func` is likely what you want. The issue with service locator is it "hides" your dependencies. If instead you take a `Func` dependency, as an example, it doesn't hide anything - it makes it crystal clear what your dependency is, and how it will be used. – mjwills Aug 04 '20 at 22:56

1 Answers1

1

The key to your answer is in the resume of your observation when profiling your application's memory: "there appears to have been 1 of each of the objects above created". Since all those objects live inside an infinite application loop, you don't have to worry about their lifetime.
From the code you've posted, the only expensive objects that are created dynamically but won't accumulate during the lifetime of Bot are the exception objects (and their associated call stacks), especially when exceptions are caught by a try-catch.

Assuming that the "Simple Injector" library you are using works properly, there is no reason to doubt the lifetime management being correctly implemented like you did. This means it only depends the way your container is configured.

Right now all your instances have a Transient lifetime, which is the default. It's important to notice this, as it appears you are expecting a Singleton lifetime.
Transient means a new instance for every request opposed to Singleton where the same shared instance is returned for each request. To achieve this behavior you must explicitly register the export with a Singleton lifetime defined:

// Container.GetInstance<IBot>() will now always return the same instance
Container.Register<IBot, Bot>(Lifestyle.Singleton);

Never use a Service Locator, especially when using Dependency Injection, just to manage an object's lifetime. As you can see, the IoC conatiner is designed to handle that. It's a key feature that is implemented by every IoC library. Service Locator can be and should be replaced by proper DI e.g., instead of passing around the IoC container you should inject abstract factories as a dependency. A direct dependency on the Service Locator introduces an unwanted tight coupling. It's very difficult to mock a dependency on a Service Locator when writing test cases.

The current implementation of Bot is also quite dangerous whe thinking about memory leaks, especially in case of the exported TelegramBotClient instance being Singleton and the EventHandler having a transient lifetime.
You hook the EventHandler to the TelegramBotClient. When the lifetime of Bot ends, you still have the TelegramBotClient keeping the EventHandler alive, which creates a memory leak. Also every new instance of Bot would attach new event handlers to the TelegramBotClient, resulting in multiple duplicate handler invocations.

To always be on the safe side you should either unsubscribe from the events immediately when they are handled or when the scopes lifetime ends e.g. in a Closed event handler or in the Dispose method. In this case make sure the object is disposed properly by client code. Since you can't always guarantee that a type like Bot is disposed properly, you should consider to create configured shared instances of the TelegramBotClient and EventHandler using an abstract factory. This factory returns a shared TelegramBotClient where all its events are observed by the shared EventHandler.
This ensures that events are subscribe to only once.

But the most preferable solution is to use the Weak-Event pattern.
You should notice this as you seem to have some trouble to determine object lifetimes and potential memory leaks. Using your code it is very easy to create a memory leak accidentally.

If you want to write robust applications, it is essential to know the main pitfalls to create memory leaks: Fighting Common WPF Memory Leaks with dotMemory, 8 Ways You can Cause Memory Leaks in .NET, 5 Techniques to avoid Memory Leaks by Events in C# .NET you should know

BionicCode
  • 1
  • 4
  • 28
  • 44
  • I greatly appreciate this in-depth answer. My original idea was indeed have TelegramBotClient, Bot, and SignalHandler all Singletons. Afterall, their objective is to sit and wait. Everything around them, in my opinion, should be more transactional and disposable. In my mind, I am attempting the approach of the AspNetCore. Web server sits and waits, each request is handled by MVC Router, and once the request is done everything associated with it goes away. – mrUlrik Aug 04 '20 at 23:09
  • 1
    Also, the introduction to Weak Events is fantastic. After reading the articles, I see that concept too has issues with potential memory leaks which I'll take into account. I've also made SignalHandler disposable to handle the unmanaged pointers--even if its lifetime is infinity, it's good practice. The explanation and articles were fantastically informative. – mrUlrik Aug 04 '20 at 23:12
  • 1
    Thank you very much. I am happy I could help! _"In my mind, I am attempting the approach of the AspNetCore. Web server sits and waits, each request is handled by MVC Router, and once the request is done everything associated with it goes away." This really depends om the scenario. – BionicCode Aug 05 '20 at 15:58
  • 1
    The point is that a usual webserver handles multiple requests of multiple clients concurrently. It's most efficient to treat each request as an atomic stateless operation - request based. Since it's stateless the server doesn't need to block/waste resources for possible future requests of a client. Instead use this resource to handle other clients requests. All objects in the context of the request would then have a `Transient` lifetime - each resource needed to serve a request only live for lifetime of the request. – BionicCode Aug 05 '20 at 15:58
  • 1
    If the server operates connection based e.g. a streaming server, then the server would indeed block the resources to create a static connection which allows an arbitrary number of requests for the lifetime of the connection e.g. socket. Resources used to serve each request would then have a `Scope` lifetime - each resource is used to handle multiple requests, but only for the lifetime of the connection. – BionicCode Aug 05 '20 at 15:58
  • 1
    When looking at your example from a service perspective then `IBot` is your service. It runs as long as the service is online and listens for requests. For each request you could create a `ITelegramBotClient` (using a factory - not the IoC container!) which only lives for the time the request is being handled. You then free this resource (or add it to a resource pool) to handle other concurrent requests. – BionicCode Aug 05 '20 at 15:59
  • 1
    If your service is some kind of messenger, then your service rather should serve requests connection based. On this case I recommend to take a close look at [SignalR](https://learn.microsoft.com/en-us/aspnet/core/signalr/introduction?view=aspnetcore-3.1). It's very simple but powerful to establish asynchronous event based real-time communication over a socket connection. – BionicCode Aug 05 '20 at 15:59
  • 1
    Generally if the resource used are expensive e.g. the service is hosted on a machine together with other services using shared memory and shared thread resources, then Singleton lifetime may lead to leave other services starved. You see the best solution is always depended on the context of your application or service. – BionicCode Aug 05 '20 at 15:59
  • Well thank you again. I was tremendously over simplifying the web server situation--and somehow it completely slipped my mind that it is stateless and everything gets disposed of by nature anyway. Regarding ITelegramBotClient: its handling the incoming requests. `IBot` is simply a wrapper to keep the process alive and start `ITelegramBotClient`. Regarding the container: I discovered [this](https://simpleinjector.readthedocs.io/en/latest/howto.html#register-factory-delegates) which I think would still not be violating any best practices. Possibly handle the `ITelegramBotClient` events. – mrUlrik Aug 05 '20 at 21:35