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

Add IsRestarting property to Umbraco application notifications #11883

Merged

Conversation

ronaldbarendse
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

This is a minor addition to PR #11857 that adds a very useful IsRestarting property to the UmbracoApplicationStartingNotification, UmbracoApplicationStartedNotification, UmbracoApplicationStoppingNotification and UmbracoApplicationStoppedNotification.

In 'normal' applications the running process is killed when it's stopped (and the application state is clean when started again), but that's not the case when Umbraco restarts: it's only stopping and starting the runtime, not the whole application.

So if you only want to execute some code on the initial startup of Umbraco, handling the UmbracoApplicationStartingNotification could result in running it twice when Umbraco has installed or upgraded (as that's when the Umbraco runtime gets restarted). In hindsight, we should've probably named these UmbracoRuntime...Notification instead 🤡

Testing can be done using the same steps as described in the related PR, but you can replace the UmbracoApplicationNotificationHandler with the class below to log the value of the new property:

public class UmbracoApplicationNotificationHandler : INotificationHandler<UmbracoApplicationStartingNotification>, INotificationHandler<UmbracoApplicationStartedNotification>, INotificationHandler<UmbracoApplicationStoppingNotification>, INotificationHandler<UmbracoApplicationStoppedNotification>
{
    private readonly ILogger _logger;

    public UmbracoApplicationNotificationHandler(ILogger<UmbracoApplicationNotificationHandler> logger) => _logger = logger;

    public void Handle(UmbracoApplicationStartingNotification notification) => Log(notification, notification.IsRestarting);

    public void Handle(UmbracoApplicationStartedNotification notification) => Log(notification, notification.IsRestarting);

    public void Handle(UmbracoApplicationStoppingNotification notification) => Log(notification, notification.IsRestarting);

    public void Handle(UmbracoApplicationStoppedNotification notification) => Log(notification, notification.IsRestarting);

    private void Log(INotification notification, bool isRestarting) => _logger.LogInformation("{Type} - {IsRestarting}", notification.GetType().Name, isRestarting);
}

Copy link
Contributor Author

@ronaldbarendse ronaldbarendse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it makes sense, we can even move the IsRestarting property to an interface and/or base class:

public interface IUmbracoApplicationLifetimeNotification : INotification
{
    bool IsRestarting { get; }
}

public abstract class UmbracoApplicationLifetimeNotificationBase : IUmbracoApplicationLifetimeNotification
{
    public UmbracoApplicationLifetimeNotificationBase(bool isRestarting = false) => IsRestarting = isRestarting;

    public bool IsRestarting { get; }
}

I'm a bit undecided on this, as it adds some inheritance/complexity, but removes some duplication (including XML docs).

@ronaldbarendse
Copy link
Contributor Author

After having another look, adding an IUmbracoApplicationLifetimeNotification interface does add some value: it conceptually groups the notifications and ensures they all use the same property type/name/documentation.

I've also updated the constructors of the notifications, so you need to explicitly pass in a value for this property. I've only left the original constructors for the existing UmbracoApplicationStartingNotification and UmbracoApplicationStoppingNotification, but obsoleted them, so they can be removed in a next major release.

The new interface also allows the UmbracoApplicationNotificationHandler (used for testing) to be a little bit cleaner too:

public class UmbracoApplicationNotificationHandler : INotificationHandler<UmbracoApplicationStartingNotification>, INotificationHandler<UmbracoApplicationStartedNotification>, INotificationHandler<UmbracoApplicationStoppingNotification>, INotificationHandler<UmbracoApplicationStoppedNotification>
{
    private readonly ILogger _logger;

    public UmbracoApplicationNotificationHandler(ILogger<UmbracoApplicationNotificationHandler> logger) => _logger = logger;

    public void Handle(UmbracoApplicationStartingNotification notification) => Log(notification);

    public void Handle(UmbracoApplicationStartedNotification notification) => Log(notification);

    public void Handle(UmbracoApplicationStoppingNotification notification) => Log(notification);

    public void Handle(UmbracoApplicationStoppedNotification notification) => Log(notification);

    private void Log(IUmbracoApplicationLifetimeNotification notification) => _logger.LogInformation("{Type} - {IsRestarting}", notification.GetType().Name, notification.IsRestarting);
}

@bergmania bergmania merged commit ca56e09 into v9/dev Jan 25, 2022
@bergmania bergmania deleted the v9/feature/umbracoapplicationnotifications-isrestarting branch January 25, 2022 05:38
bergmania pushed a commit that referenced this pull request Jan 25, 2022
* Add IsRestarting property to Umbraco application notifications

* Add IUmbracoApplicationLifetimeNotification and update constructors

* Only subscribe to events on initial startup

* Cleanup CoreRuntime

* Do not reset StaticApplicationLogging instance after stopping/during restart
@bergmania
Copy link
Member

Cherry picked to 9.3rc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants