Skip to content

Await the deployed instances refresh instead of firing it as async void #5768

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.

Sign up for GitHub

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

Merged
johnsimons merged 1 commit into from
Aug 13, 2026
Merged

Await the deployed instances refresh instead of firing it as async void #5768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/ServiceControl.Config.Tests/InstanceDetails/CorruptInstanceConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())
{
Expand All @@ -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())
{
Expand Down Expand Up @@ -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<ListInstancesViewModel> 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
{
Expand Down
14 changes: 4 additions & 10 deletions src/ServiceControl.Config/UI/ListInstances/ListInstancesViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ internal ListInstancesViewModel(Func<BaseService, InstanceDetailsViewModel> inst
CopyToClipboard = new CopyToClipboardCommand();

Instances = [];

AddAndRemoveInstances();
}

protected override Task OnInitialize(CancellationToken cancellationToken = default) => AddAndRemoveInstances(cancellationToken);

public CopyToClipboardCommand CopyToClipboard { get; }

public BindableCollection<InstanceDetailsViewModel> OrderedInstances => [.. Instances.OrderBy(x => x.Name)];
Expand Down Expand Up @@ -107,7 +107,7 @@ public Task HandleAsync(LicenseUpdated licenseUpdatedEvent, CancellationToken ca
/// </summary>
public async Task HandleAsync(RefreshInstances message, CancellationToken cancellationToken = default)
{
AddAndRemoveInstances();
await AddAndRemoveInstances(cancellationToken);
await EventAggregator.PublishOnUIThreadAsync(new PostRefreshInstances(), cancellationToken);
}

Expand All @@ -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();
Expand Down Expand Up @@ -169,7 +164,6 @@ async void AddAndRemoveInstances()
NotifyOfPropertyChange(nameof(HasConfigurationErrors));
NotifyOfPropertyChange(nameof(ConfigurationErrorMessage));
}
#pragma warning restore PS0027

readonly Func<BaseService, InstanceDetailsViewModel> instanceDetailsFunc;
readonly Func<IEnumerable<BaseService>> getAllInstances;
Expand Down