CShark-Hub / Mako-IoT.Home

🏠 The landing page for MakoIoT project.
Other
0 stars 0 forks source link

[Feature Request] Support multiple 'IDeviceStartBehavior' with deterministic execution order #64

Closed CoryCharlton closed 12 months ago

CoryCharlton commented 1 year ago

This is related to https://github.com/CShark-Hub/Mako-IoT.Home/issues/63

Currently only a single 'IDeviceStartBehavior' is supported.

It may be beneficial to support multiple 'IDeviceStartBehavior' but we would need to ensure that the order of execution is controller.

I'm opening this feature request for feedback on whether this should be supported and to discuss potential solutions.

CoryCharlton commented 1 year ago

Proposed solution:

Add a new configuration 'IDeviceStartBehaviorConfiguration'.

public interface IDeviceStartBehaviorConfiguration
{
    // Default to false or true?
    public bool AllowNonDeterministicExecution { get; set; }

    // List of concrete implementations of IDeviceStartBehavior
    public ArrayList ExecutionOrder { get; set; }
}

If more than one 'IDeviceStartBehavior' is found then it would be required for an instance of 'IDeviceStartBehaviorConfiguration' to be registered as well.

If AllowNonDeterministicExecution is true then IotDevice would just execute them in whatever order comes back from the ServiceProvider

If AllowNonDeterministicExecution is false then IotDevice would require that ExecutionOrder contains all the 'IDeviceStartBehavior' registered and would execute them in that order.

torbacz commented 1 year ago

I totally agree that we should support multiple "IDeviceStartingBehaviours" and that execution order is critical. To maintain that order we may relay on NF dependency injection, which at first glance should maintain order. So in my opinion, this one should be pretty easy to implement, as DI already supports returning array of objects.

https://github.com/nanoframework/nanoFramework.DependencyInjection

EDIT: OF course, we should implement some unit tests to check if the execution order is correct.

CoryCharlton commented 12 months ago

@torbacz looking at https://github.com/nanoframework/nanoFramework.DependencyInjection the ServiceCollection uses an ArrayList to store the descriptors:

https://github.com/nanoframework/nanoFramework.DependencyInjection/blob/f43b8b3b77b63248768553d7ead7ca77ed56fdd2/nanoFramework.DependencyInjection/DependencyInjection/ServiceCollection.cs#L16

And then uses another ArrayList when returning them:

https://github.com/nanoframework/nanoFramework.DependencyInjection/blob/f43b8b3b77b63248768553d7ead7ca77ed56fdd2/nanoFramework.DependencyInjection/DependencyInjection/ServiceProviderEngine.cs#L129C7-L129C7

My quick testing also shows that they are returned in the order they were registered (with the exception of the scopedServices parameter which I didn't dig into).

The "order they were registered" is the key here. Consider the following example:

    public class Program
    {
        public static void Main()
        {
            var deviceA = BuildDeviceA();
            var deviceB = BuildDeviceB();
            var deviceC = BuildDeviceC();

            var servicesA = deviceA.ServiceProvider;
            var servicesB = deviceB.ServiceProvider;
            var servicesC = deviceC.ServiceProvider;

            var deviceStartBehaviorsA = servicesA.GetServices(typeof(IDeviceStartBehavior));
            var deviceStartBehaviorsB = servicesB.GetServices(typeof(IDeviceStartBehavior));

            // Success
            var deviceStartBehaviorsC = servicesC.GetServices(typeof(IDeviceStartBehavior));
        }

        public static IDevice BuildDeviceA()
        {
            var builder = DeviceBuilder.Create();

            // I need TestDeviceStartBehavior to run first thing so I register it
            builder.ConfigureDI(services =>
            {
                services.AddSingleton(typeof(IDeviceStartBehavior), typeof(TestDeviceStartBehavior));
                // ... Other registrations
            });

            // At first glance I don't realize that AddConfigurationManager() also adds a IDeviceStartBehavior
            builder.AddConfigurationManager();
            builder.AddFileStorage();
            builder.AddLogging();

            return builder.Build();
        }

        public static IDevice BuildDeviceB()
        {
            var builder = DeviceBuilder.Create();

            builder.AddConfigurationManager();
            builder.AddFileStorage();
            builder.AddLogging();

            builder.ConfigureDI(services =>
            {
                services.AddSingleton(typeof(IDeviceStartBehavior), typeof(TestDeviceStartBehavior));
                // ... Other registrations
            });

            return builder.Build();
        }

        public static IDevice BuildDeviceC()
        {
            var builder = DeviceBuilder.Create();

            builder.Services.AddSingleton(typeof(IDeviceStartBehavior), typeof(TestDeviceStartBehavior));

            builder.AddConfigurationManager();
            builder.AddFileStorage();
            builder.AddLogging();

            builder.ConfigureDI(services =>
            {
                // ... Other registrations
            });

            return builder.Build();
        }
    }

    public class TestDeviceStartBehavior: IDeviceStartBehavior
    {
        public bool DeviceStarting()
        {
            return true;
        }
    }

In both BuildDeviceA() and BuildDeviceB() the TestDeviceStartBehavior comes last because ConfigureDI is a delayed action. So at first glance BuildDeviceA() appears that TestDeviceStartBehavior will be the first IDeviceStartBehavior but it's not and the user has to dig a bit to determine why.

Maybe I'm overthinking this and the naïve implementation (foreach GetServices() in Start), requiring the user to have the ability to work out how things are actually working, is an acceptable solution (IIRC this is how Middleware in ASP.NET Core handles things) :shrug:

CoryCharlton commented 12 months ago

After sleeping on this I think I am over complicating this so I'll send over a PR for the simple solution shortly.

torbacz commented 12 months ago

Completed