Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Startup init for singletons #15

Open
dicko2 opened this issue Mar 5, 2022 · 7 comments
Open

Startup init for singletons #15

dicko2 opened this issue Mar 5, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@dicko2
Copy link
Contributor

dicko2 commented Mar 5, 2022

Autofac has this

    /// <summary>
    /// When implemented by a component, an instance of the component will be resolved
    /// and started as soon as the container is built. Autofac will not call the Start()
    /// method when subsequent instances are resolved. If this behavior is required, use
    /// an <c>OnActivated()</c> event handler instead.
    /// </summary>
    /// <remarks>
    /// For equivalent "Stop" functionality, implement <see cref="IDisposable"/>. Autofac
    /// will always dispose a component before any of its dependencies (except in the presence
    /// of circular dependencies, in which case the components in the cycle are disposed in
    /// reverse-construction order.)
    /// </remarks>
    public interface IStartable
    {
        /// <summary>
        /// Perform once-off startup processing.
        /// </summary>
        void Start();
    }

anything that inherits from it will init on startup.

It's a cool idea some something we use in Supply extranets at Agoda. There this IHostedService now as well for background workers too

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-6.0&tabs=visual-studio

Idea would be to do something like this if we follow the autofac approach

[RegisterSingleton]
public class MyBackgroundWorker : IMyBackgroundWorker, IStartupable
{
// ..
} 

And then call a GetService after app init, this would also require use to add a new extension method though something like "UseAgodaIoC", or more specific "StartupAgodaIoCSingletons".

Alternatively we could simply leverage the IHostedService and register them with a new attribute

[RegisterSingletonStartup]
public class MyBackgroundWorker : IHostedService
{
// ..
} 

// in registartion
services.AddHostedService<MyBackgroundWorker>();
//..

The problem here though is that AddHostedService has no support for registering an interface.

In most of our use cases the startup method is prewarming cache in singletons that are later used for data fetchnig (i.e. Repositories, etc). So i think this wont work for us as we need interfaced use in teh regestration to properly mock for unit testing, so we need something simple like the autofac example imo.

Thoughts?

@dicko2 dicko2 added the enhancement New feature or request label Mar 5, 2022
@dicko2
Copy link
Contributor Author

dicko2 commented Mar 6, 2022

Adding some context from slack

A suggestion was made to use a background worker to prewarm anything that needs prewarming.

Essentially having a central "warmup class"

I'm not against people doing this, but one of the principles that we built agoda ioc on was decentralization of responsibilities, The 4k line UnityConfiguration class in the Agoda website from many years ago comes to mind.

When we look at the example in one of the larger supply extranets where we want to apply this, there's about more than 20 services/repos that need prewarm (a lot less than unity configuration nightmare for sure), then i think about the unit test we would need to write.

var underTest = new PrewarmService(MockGeoRepository,MockLanguageRepository, ...);// passing in >20 mocks
// we also have static content checks that this would fail because the ctor has too many parameters
MockGeoRepository.Prewarm().Received(1);
MockLanguageRepository.Prewarm().Received(1);
// copy past this type of line 20 times

Maybe I am missing something, but i feel this is less elegant than the autofac solution.

@szaboopeeter
Copy link
Member

szaboopeeter commented Mar 6, 2022

I find the [RegisterSingletonStartup] approach quite limiting (you can only use it with hosted services), especially compared to what you can achieve with Autofac.

It even feels less "safe" compared just using the service collection extensions directly, because AddHostedService<THostedService>() has a type constraint for the generic which ensures compile time errors if you don't use it with an IHostedService implementation - but you can drop in the attribute on any class and potentially blow up in runtime (we can probably write analyzers for that though...).

@dicko2
Copy link
Contributor Author

dicko2 commented Mar 6, 2022

So @szaboopeeter you are suggesting we do something like

[RegisterSingleton]
public class MyRepoThatNeedsWarmup : IMyRepoThatNeedsWarmup, IStartupable
{
// ..
} 

As the approach, so it's just a singleton in terms of attribute, and we infer the Startup feature by the fact it is inheriting from IStartupable?

@kchinburarat
Copy link
Contributor

kchinburarat commented Mar 8, 2022

``This is my idea

Let's say on consumer of Agoda.Ioc have these class

[RegisterSingleton]
public class MyRepoThatNeedsWarmup1 : IMyRepoThatNeedsWarmup1, IStartableAsync
{
    // IStartableAsync implementation
     public async Task StartAsync(){
         // Do warmup 
     }
} 


[RegisterSingleton]
public class MyRepoThatNeedsWarmup2 : IMyRepoThatNeedsWarmup2, IStartableAsync
{
    // IStartableAsync implementation
     public async Task StartAsync(){
         // Do warmup 
     }
} 

All of these class were register as IStartableAsync

In Agoda.Ioc we provide new IHostedService called NeedsWarmupBackgroundWorker

    /// <summary>
    /// This service was registered by Agoda.Ioc if comsumer has IStartableAsync implementation class
    /// services.AddHostedService<NeedsWarmupBackgroundWorker>();
    /// </summary>

    public class NeedsWarmupBackgroundWorker : IHostedService
    {
        private readonly IEnumerable<IStartableAsync> _startableAsyncs;
        // inject all singleto services
        public cBackgroundWorker(IEnumerable<IStartableAsync> startableAsyncs)
        {
            _startableAsyncs = startableAsyncs;
        }
        public async Task StartAsync(CancellationToken cancellationToken)
        {
            foreach (var startableAsync in _startableAsyncs)
            {
                await startableAsync.StartAsync();
            }
        }
        public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask;
    }

This way before we start we can make sure every NeedsWarmup class will be done before app start

@dicko2
Copy link
Contributor Author

dicko2 commented Mar 8, 2022

@kchinburarat when is

StartAsync

Called? It's after app.Run()? Or no?

@kchinburarat
Copy link
Contributor

kchinburarat commented Mar 8, 2022

I think it will start on app.run and should finish before kestrel do port binding

@kchinburarat
Copy link
Contributor

This is the execution step
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants