From 64c70a5c3ce31938af967a3139d292cbce485444 Mon Sep 17 00:00:00 2001 From: John Simons Date: Wed, 12 Aug 2026 17:50:13 +1000 Subject: [PATCH] Await the deployed instances refresh instead of firing it as async void ListInstancesViewModel.AddAndRemoveInstances was a genuine async void, not an event handler. It returned at its first await, so HandleAsync(RefreshInstances) published PostRefreshInstances while the removals were still in flight. That is the exact ordering the method's own remarks say must not happen: deleting an instance in PowerShell could then error out a deleted instance viewmodel trying to refresh itself. It is now async Task and awaited. The only thing that kept it async void was the constructor call site, which cannot await; that call moves to RxScreen.OnInitialize, which takes a token and is awaited by IActivate.ActivateAsync. The list therefore populates on activation rather than construction. The CorruptInstanceConfiguration specs construct the viewmodel directly, so they now activate it before asserting. Deferred since Phase 0 of the cancellation work, and unblocked by OnInitialize gaining a CancellationToken. --- .../CorruptInstanceConfiguration.cs | 24 ++++++++++++------- .../ListInstances/ListInstancesViewModel.cs | 14 ++++------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/ServiceControl.Config.Tests/InstanceDetails/CorruptInstanceConfiguration.cs b/src/ServiceControl.Config.Tests/InstanceDetails/CorruptInstanceConfiguration.cs index 5245eac65a..c96a82ac60 100644 --- a/src/ServiceControl.Config.Tests/InstanceDetails/CorruptInstanceConfiguration.cs +++ b/src/ServiceControl.Config.Tests/InstanceDetails/CorruptInstanceConfiguration.cs @@ -253,6 +253,7 @@ public async Task The_one_where_the_fix_is_picked_up_through_the_deployed_instan { EventAggregator = new EventAggregator() }; + await ((IActivate)list).ActivateAsync(TestContext.CurrentContext.CancellationToken); Assert.That(list.HasConfigurationErrors, Is.True, "Precondition: the list starts out with a corrupt instance"); // The operator fixes the file, then triggers the refresh the UI uses @@ -294,12 +295,12 @@ public void The_one_where_a_refresh_tries_to_apply_data_from_an_instance_of_a_di public class Rule_5_Must_summarize_configuration_errors_above_the_instance_list : CorruptInstanceConfigurationFixture { [Test] - public void The_one_where_a_single_instance_is_corrupt_and_the_banner_names_it() + public async Task The_one_where_a_single_instance_is_corrupt_and_the_banner_names_it() { WriteErrorInstanceConfig(CorruptXml); WriteAuditInstanceConfig(ValidAuditInstanceXml); - var list = ListFor(LoadErrorInstance(), LoadAuditInstance("Particular.ServiceControl.Audit")); + var list = await ListFor(LoadErrorInstance(), LoadAuditInstance("Particular.ServiceControl.Audit")); using (Assert.EnterMultipleScope()) { @@ -310,23 +311,23 @@ public void The_one_where_a_single_instance_is_corrupt_and_the_banner_names_it() } [Test] - public void The_one_where_multiple_instances_are_corrupt_and_the_banner_lists_all_of_them() + public async Task The_one_where_multiple_instances_are_corrupt_and_the_banner_lists_all_of_them() { WriteErrorInstanceConfig(CorruptXml); WriteAuditInstanceConfig(CorruptXml); - var list = ListFor(LoadErrorInstance(), LoadAuditInstance("Particular.ServiceControl.Audit")); + var list = await ListFor(LoadErrorInstance(), LoadAuditInstance("Particular.ServiceControl.Audit")); Assert.That(list.ConfigurationErrorMessage, Is.EqualTo("Multiple instances (Particular.ServiceControl, Particular.ServiceControl.Audit) cannot be loaded due to XML configuration errors.")); } [Test] - public void The_one_where_all_configurations_are_valid_and_no_banner_is_shown() + public async Task The_one_where_all_configurations_are_valid_and_no_banner_is_shown() { WriteErrorInstanceConfig(ValidErrorInstanceXml); - var list = ListFor(LoadErrorInstance()); + var list = await ListFor(LoadErrorInstance()); using (Assert.EnterMultipleScope()) { @@ -400,8 +401,15 @@ protected MonitoringInstance LoadMonitoringInstance(string serviceName = Service internal static InstanceDetailsViewModel DetailsFor(BaseService instance) => new(instance, null, null, null, null, null, null, null, null); - internal static ListInstancesViewModel ListFor(params BaseService[] instances) => - new(DetailsFor, () => instances); + // The list populates in OnInitialize, so it has to be activated before it has anything in it +#pragma warning disable PS0018 // A params array must be the last parameter, so a trailing CancellationToken cannot be added + internal static async Task ListFor(params BaseService[] instances) +#pragma warning restore PS0018 + { + var list = new ListInstancesViewModel(DetailsFor, () => instances); + await ((IActivate)list).ActivateAsync(TestContext.CurrentContext.CancellationToken); + return list; + } class FakeWindowsServiceController(string exePath, string serviceName) : IWindowsServiceController { diff --git a/src/ServiceControl.Config/UI/ListInstances/ListInstancesViewModel.cs b/src/ServiceControl.Config/UI/ListInstances/ListInstancesViewModel.cs index bb89720cd6..6ec76f53f2 100644 --- a/src/ServiceControl.Config/UI/ListInstances/ListInstancesViewModel.cs +++ b/src/ServiceControl.Config/UI/ListInstances/ListInstancesViewModel.cs @@ -32,10 +32,10 @@ internal ListInstancesViewModel(Func inst CopyToClipboard = new CopyToClipboardCommand(); Instances = []; - - AddAndRemoveInstances(); } + protected override Task OnInitialize(CancellationToken cancellationToken = default) => AddAndRemoveInstances(cancellationToken); + public CopyToClipboardCommand CopyToClipboard { get; } public BindableCollection OrderedInstances => [.. Instances.OrderBy(x => x.Name)]; @@ -107,7 +107,7 @@ public Task HandleAsync(LicenseUpdated licenseUpdatedEvent, CancellationToken ca /// public async Task HandleAsync(RefreshInstances message, CancellationToken cancellationToken = default) { - AddAndRemoveInstances(); + await AddAndRemoveInstances(cancellationToken); await EventAggregator.PublishOnUIThreadAsync(new PostRefreshInstances(), cancellationToken); } @@ -127,12 +127,7 @@ public async Task HandleAsync(ResetInstances message, CancellationToken cancella NotifyOfPropertyChange(nameof(Instances)); } - // TODO: this is a genuine async void, not an event handler. Because it returns at the first - // await, HandleAsync(RefreshInstances) publishes PostRefreshInstances before the removals have - // finished, which is the ordering that method's own remarks say must not happen. Converting it - // to async Task needs the constructor call site at the top of this class restructured first. -#pragma warning disable PS0027 - async void AddAndRemoveInstances() + async Task AddAndRemoveInstances(CancellationToken cancellationToken) { // Remove instances that no longer exist on disk var toRemove = Instances.Where(instance => !instance.Exists()).ToList(); @@ -169,7 +164,6 @@ async void AddAndRemoveInstances() NotifyOfPropertyChange(nameof(HasConfigurationErrors)); NotifyOfPropertyChange(nameof(ConfigurationErrorMessage)); } -#pragma warning restore PS0027 readonly Func instanceDetailsFunc; readonly Func> getAllInstances;