Restore triggers too early for 3rd party add-ons

[mattbrailsford]

There is an issue with Deploy triggering too early if there are third party add-ons that require their IComponent implementations to run before they can safely deploy artefacts.

Reproduction

Bug summary

It appears that Umbraco.Deploy triggers a deploy from within it's IComponent.Initialize method, however because the DeployComposer class is registered before IUserComposer instances I believe this means that it is composed and thus initialized before all other composers / components of third parties.

The issue is, Vendr requires it's IComponent to be initialized before it can safely execute as there are a number of setup steps that occur within that method that it relies upon. Because these aren't triggered though before Deploy attempts a restore the attempt fails with exceptions due to a number of resources being null.

Steps to reproduce

• Create a cloud site
• Install Vendr + Vendr.Deploy
• Setup a store in the settings section on the dev env
• Pull the site locally and restore (this should succeed)
• Trigger another restore by creating a deploy file in the data directory and then touch the web.config
• The deploy will now fail because the deploy occurred before Vendr's IComponent was initialized which contains essential setup steps for Vendr to run

Expected result

I would expect that Umbraco Deploy should wait until the application is fully initialized before it attempts a restore as it can't know what third party dependencies it has and whether those dependencies are ready yet or not.

Actual result

Deploy attempts a restore too early and so causes errors.

[AndyButland]

Yes, I think I can see the issue here. We have a DeployComposer set up to run after core but before user composers - that does various things, including starting up the processing of existing or new file system triggers. It looks to me that we should be splitting that up, so we remove that file system initialization to a different composer, configured to run after all the IUserComposer instances. I've created a PR for this so we'll look to get it reviewed and into the next release.

[MichaelNielsenDK]

This is mentioned to be fixed in Deploy 4.4.2.

Is it also fixed in v9, and if so in what release? Because I'm having the same issue on a v9 site.

[AndyButland]

It's not in V9 no @MichaelNielsenDK, but only because I didn't think the issue was found there. It's possible I missed something though. If you can share a cut down example of the problem you are running into will be happy to look further.

[MichaelNielsenDK]

@AndyButland I've described it here
vendrhub/vendr-deploy#11

And @mattbrailsford defined it to be the same issue as
vendrhub/vendr-deploy#10
which is the base issue for this issue.

But let me know if you need more information.

[AndyButland]

Thanks - I'll have a further look at this.

[AndyButland]

Hi @MichaelNielsenDK and @mattbrailsford - have had another look at this with Deploy V9, and it does seem like we don't have the same issue as was the case in V8, and needed the recent patch update. Or at least and I can't see it my tests, and so wanted to replay what I've tried to see if you can see what I'm missing.

In V8 we had the issue because Deploy was running it's first extraction based on finding a deploy marker file on disk as part of start-up, triggered from a composer/component that didn't have any timing configured. As such this could run before or after any other composers/components, and if it ran before, it would cause the problem - as other composer/components, including the Vendr Deploy one, wouldn't have run their necessary setup. This was resolved by creating a second composer/component for starting the extractions triggered by disk files, and ensuring that runs after all the other composers.

In V9 though, the disk file monitoring and extractions are set up in a handler of the UmbracoApplicationStartingNotification, and that does seem to run after the composers.

To test this I've created a simple composer/component:

    public class TestSiteComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            builder.Components()
                .Append<TestSiteComponent>();
        }
    }

    public class TestSiteComponent : IComponent
    {
        private readonly ILogger<TestSiteComponent> _logger;

        public TestSiteComponent(ILogger<TestSiteComponent> logger)
        {
            _logger = logger;
        }

        public void Initialize()
        {
            _logger.LogInformation($"Intializing {nameof(TestSiteComponent)}");
        }

        public void Terminate()
        {
        }
    }

Via breakpoints I can see that the Initialize() method is called before the handler of the UmbracoApplicationStartingNotification is. Hence I'd expect any setup to be in place before we ran any extractions triggered from the marker files.

I suspect if you are still seeing a problem it's because of something I'm not replicating properly in my tests. I'm thinking perhaps if Vendr (or anything else) was doing setup in a UmbracoApplicationStartingNotification handler, we might have a similar race condition. But if you are able to confirm that, or otherwise shed any light on what that might, that would be appreciated. Thanks.

[mattbrailsford]

So in Vendr v2 for v9 we do have a few UmbracoApplicationStartingNotification handlers. One to extend the lucene index with some optimizations for variant data, one to enable Vendr's cache refreshers and one to run our DB migrations. Something that might also be a problem (and I believe would result in the same error we were getting in v8) is that we also have an IStartupFilter which is responsible for running some of the code that the v8 IComponent was doing so maybe again, this is running after the Umbraco code is running?

[AndyButland]

Could be - are you able to share a cut down version of what's going on in your IStartupFilter please? E.g. perhaps just with log lines, so I can hook something similar up to my test project and see the order of events?

[mattbrailsford]

    public class VendrStartupFilter : IStartupFilter
    {
        public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
            => (app) =>
            {
                // Set the service locator factory
                ServiceLocator.Factory = app.ApplicationServices.GetRequiredService<IFactory>();

                // Set the GUID Provider
                var guidProvider = (IGuidProvider)ServiceLocator.Factory.TryGetInstance(typeof(IGuidProvider));
                if (guidProvider != null)
                {
                    GuidFactory.GuidProvider = guidProvider;
                }

                next(app);
            };
      }
  }

This is basically my start-up filter which I believe it'll be our ServiceLocator that is likely the problem as anything that needs to use it won't have it available yet if it is running too late.

[AndyButland]

Thanks. It does look as if this code will run after the UmbracoApplicationStartingNotification notification is fired. Umbraco is in control over this notification and the component initialization, so that part is OK - all components are initialized before this notification is published. But I'm not sure if we have a way of running code after all IStartUpFilters are run - will have a think and ask around.

I wonder though if you could avoid needing this filter. I assume it's there so you can statically access the IGuidProvider in a context where you either can't or don't want to inject it, like your SetName() extension method?

If so, there's is a feature in V9 to do service location. It's not encouraged, but is available and justified to use for two reasons - avoiding breaking changes to signatures and to keep "friendly" extension methods available - both of which seem to apply here.

It seems you could define GuidProvider as:

    public static class GuidFactory
    {
        private static IGuidProvider GuidProvider { get; } =
            StaticServiceProvider.Instance.GetRequiredService<IGuidProvider ();
        ...

And then rather then setting it in the startup filter, you could just access this now read-only property and use the provider via service location. There's an example of Umbraco doing this here.

StaticServiceProvider was made public in 9.0.1, so if you are referencing 9.0.0 you won't be able to access it.

As I say I'll see if there's anything we can amend in Deploy here to still try to get these file triggers processed after everything has started up, but it could be this approach will allow you to fix the issue on the Vendr side.

[mattbrailsford]

Hey @AndyButland, I could yea, but given some history, I'm basically trying to avoid as much Umbraco dependency as I can as then everything becomes out of my control.

To be fair though, I only implemented it this way because that's the best way I could find to do it. If there is some other way to make sure this gets fired on application startup before my other code I'm sure that would be fine 👍