commit f6062a574d2243d3a38e5764cd48b558a5bbf9bb
parent 34fbaabe25df5353d92333dbe2c968a5a2feb316
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Wed, 31 Jan 2024 15:01:15 -0800
Improve progress handling for group processor (#4121)
To prevent the loss of progress events between the group processor and
the caller, move to a model where these internal interfaces take in an
`EventHandler` parameter rather than using `...WithProgress`. This will
make it possible to be confident that no progress events occurred
between the async method starting and the progress handler being
attached.
This fixes the flakiness of the tests that were verifying progress
events.
Diffstat:
9 files changed, 91 insertions(+), 62 deletions(-)
diff --git a/src/AppInstallerSharedLib/Public/winget/AsyncTokens.h b/src/AppInstallerSharedLib/Public/winget/AsyncTokens.h
@@ -118,6 +118,29 @@ namespace AppInstaller::WinRT
private:
Token m_token;
};
+
+ // Type containing winrt EventHandler.
+ template <typename ResultT, typename ProgressT>
+ struct AsyncProgressEventHandlerT : public AsyncProgressTypeErasure<ResultT, ProgressT>
+ {
+ using Token = winrt::Windows::Foundation::EventHandler<ProgressT>;
+
+ AsyncProgressEventHandlerT(Token&& token) : m_token(std::move(token)) {}
+
+ void Progress(ProgressT const& progress) const override
+ {
+ m_token(m_result, progress);
+ }
+
+ void Result(ResultT const& result) const override
+ {
+ m_result = result;
+ }
+
+ private:
+ mutable ResultT m_result = nullptr;
+ Token m_token;
+ };
}
// May hold a progress token and provide the ability to send progress updates.
@@ -136,6 +159,14 @@ namespace AppInstaller::WinRT
m_token = std::make_unique<details::AsyncProgressT<Promise, ResultT, ProgressT>>(std::move(progress));
}
+ // Create a progress object from an EventHandler.
+ template <typename Promise>
+ AsyncProgress(winrt::Windows::Foundation::EventHandler<ProgressT>&& progress, winrt::impl::cancellation_token<Promise>&& cancellation) :
+ AsyncCancellation(std::move(cancellation))
+ {
+ m_token = std::make_unique<details::AsyncProgressEventHandlerT<ResultT, ProgressT>>(std::move(progress));
+ }
+
// Sends progress if this object is not empty.
void Progress(ProgressT const& progress) const
{
diff --git a/src/Microsoft.Management.Configuration.UnitTests/Helpers/TestConfigurationSetGroupProcessor.cs b/src/Microsoft.Management.Configuration.UnitTests/Helpers/TestConfigurationSetGroupProcessor.cs
@@ -50,10 +50,11 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
/// <summary>
/// Apply settings for the group.
/// </summary>
+ /// <param name="progressHandler">The progress handler.</param>
/// <returns>The operation to apply settings.</returns>
- public IAsyncOperationWithProgress<IApplyGroupSettingsResult, IApplyGroupMemberSettingsResult> ApplyGroupSettingsAsync()
+ public IAsyncOperation<IApplyGroupSettingsResult> ApplyGroupSettingsAsync(EventHandler<IApplyGroupMemberSettingsResult> progressHandler)
{
- return AsyncInfo.Run((CancellationToken cancellationToken, IProgress<IApplyGroupMemberSettingsResult> progress) => Task.Run<IApplyGroupSettingsResult>(() =>
+ return AsyncInfo.Run((CancellationToken cancellationToken) => Task.Run<IApplyGroupSettingsResult>(() =>
{
this.WaitOnAsyncEvent(cancellationToken);
@@ -62,7 +63,7 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
if (this.Set != null)
{
- TestConfigurationUnitGroupProcessor.ApplyGroupSettings(this.Set.Units, progress, result);
+ TestConfigurationUnitGroupProcessor.ApplyGroupSettings(this.Set.Units, progressHandler, result);
}
return result;
@@ -72,10 +73,11 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
/// <summary>
/// Test settings for the group.
/// </summary>
+ /// <param name="progressHandler">The progress handler.</param>
/// <returns>The operation to test settings.</returns>
- public IAsyncOperationWithProgress<ITestGroupSettingsResult, ITestSettingsResult> TestGroupSettingsAsync()
+ public IAsyncOperation<ITestGroupSettingsResult> TestGroupSettingsAsync(EventHandler<ITestSettingsResult> progressHandler)
{
- return AsyncInfo.Run((CancellationToken cancellationToken, IProgress<ITestSettingsResult> progress) => Task.Run<ITestGroupSettingsResult>(() =>
+ return AsyncInfo.Run((CancellationToken cancellationToken) => Task.Run<ITestGroupSettingsResult>(() =>
{
this.WaitOnAsyncEvent(cancellationToken);
@@ -85,7 +87,7 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
if (this.Set != null)
{
result.TestResult = TestConfigurationUnitGroupProcessor.GetTestResult(this.Set.Metadata);
- TestConfigurationUnitGroupProcessor.TestGroupSettings(this.Set.Units, progress, result);
+ TestConfigurationUnitGroupProcessor.TestGroupSettings(this.Set.Units, progressHandler, result);
}
return result;
diff --git a/src/Microsoft.Management.Configuration.UnitTests/Helpers/TestConfigurationUnitGroupProcessor.cs b/src/Microsoft.Management.Configuration.UnitTests/Helpers/TestConfigurationUnitGroupProcessor.cs
@@ -55,17 +55,18 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
/// <summary>
/// Apply settings for the group.
/// </summary>
+ /// <param name="progressHandler">The progress handler.</param>
/// <returns>The operation to apply settings.</returns>
- public IAsyncOperationWithProgress<IApplyGroupSettingsResult, IApplyGroupMemberSettingsResult> ApplyGroupSettingsAsync()
+ public IAsyncOperation<IApplyGroupSettingsResult> ApplyGroupSettingsAsync(EventHandler<IApplyGroupMemberSettingsResult> progressHandler)
{
- return AsyncInfo.Run((CancellationToken cancellationToken, IProgress<IApplyGroupMemberSettingsResult> progress) => Task.Run<IApplyGroupSettingsResult>(() =>
+ return AsyncInfo.Run((CancellationToken cancellationToken) => Task.Run<IApplyGroupSettingsResult>(() =>
{
this.WaitOnAsyncEvent(cancellationToken);
ApplyGroupSettingsResultInstance result = new (this.Group);
result.UnitResults = new List<IApplyGroupMemberSettingsResult>();
- ApplyGroupSettings(this.Unit.Units, progress, result);
+ ApplyGroupSettings(this.Unit.Units, progressHandler, result);
return result;
}));
@@ -74,10 +75,11 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
/// <summary>
/// Test settings for the group.
/// </summary>
+ /// <param name="progressHandler">The progress handler.</param>
/// <returns>The operation to test settings.</returns>
- public IAsyncOperationWithProgress<ITestGroupSettingsResult, ITestSettingsResult> TestGroupSettingsAsync()
+ public IAsyncOperation<ITestGroupSettingsResult> TestGroupSettingsAsync(EventHandler<ITestSettingsResult> progressHandler)
{
- return AsyncInfo.Run((CancellationToken cancellationToken, IProgress<ITestSettingsResult> progress) => Task.Run<ITestGroupSettingsResult>(() =>
+ return AsyncInfo.Run((CancellationToken cancellationToken) => Task.Run<ITestGroupSettingsResult>(() =>
{
this.WaitOnAsyncEvent(cancellationToken);
@@ -85,7 +87,7 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
result.UnitResults = new List<ITestSettingsResult>();
result.TestResult = GetTestResult(this.Unit.Metadata);
- TestGroupSettings(this.Unit.Units, progress, result);
+ TestGroupSettings(this.Unit.Units, progressHandler, result);
return result;
}));
@@ -126,16 +128,17 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
/// <param name="groupMembers">The group members.</param>
/// <param name="progress">The progress reporting object.</param>
/// <param name="result">The result object.</param>
- internal static void ApplyGroupSettings(IList<ConfigurationUnit>? groupMembers, IProgress<IApplyGroupMemberSettingsResult> progress, ApplyGroupSettingsResultInstance result)
+ internal static void ApplyGroupSettings(IList<ConfigurationUnit>? groupMembers, EventHandler<IApplyGroupMemberSettingsResult> progress, ApplyGroupSettingsResultInstance result)
{
if (groupMembers != null)
{
foreach (ConfigurationUnit unit in groupMembers)
{
ApplyGroupMemberSettingsResultInstance unitResult = new (unit);
+ result.UnitResults!.Add(unitResult);
unitResult.State = ConfigurationUnitState.InProgress;
- progress.Report(unitResult);
+ progress.Invoke(null, unitResult);
unitResult.PreviouslyInDesiredState = GetTestResult(unit) == ConfigurationTestResult.Positive;
@@ -145,9 +148,7 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
}
unitResult.State = ConfigurationUnitState.Completed;
- progress.Report(unitResult);
-
- result.UnitResults!.Add(unitResult);
+ progress.Invoke(null, unitResult);
}
}
}
@@ -158,7 +159,7 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
/// <param name="groupMembers">The group members.</param>
/// <param name="progress">The progress reporting object.</param>
/// <param name="result">The result object.</param>
- internal static void TestGroupSettings(IList<ConfigurationUnit>? groupMembers, IProgress<ITestSettingsResult> progress, TestGroupSettingsResultInstance result)
+ internal static void TestGroupSettings(IList<ConfigurationUnit>? groupMembers, EventHandler<ITestSettingsResult> progress, TestGroupSettingsResultInstance result)
{
if (groupMembers != null)
{
@@ -172,9 +173,8 @@ namespace Microsoft.Management.Configuration.UnitTests.Helpers
}
unitResult.TestResult = GetTestResult(unit);
- progress.Report(unitResult);
-
result.UnitResults!.Add(unitResult);
+ progress.Invoke(null, unitResult);
}
}
}
diff --git a/src/Microsoft.Management.Configuration.UnitTests/Tests/ConfigurationProcessorGroupTests.cs b/src/Microsoft.Management.Configuration.UnitTests/Tests/ConfigurationProcessorGroupTests.cs
@@ -9,6 +9,7 @@ namespace Microsoft.Management.Configuration.UnitTests.Tests
using System;
using System.Collections.Generic;
using System.Linq;
+ using System.Threading;
using Microsoft.Management.Configuration.UnitTests.Fixtures;
using Microsoft.Management.Configuration.UnitTests.Helpers;
using Xunit;
@@ -416,16 +417,19 @@ resources:
configurationSet.Units = new ConfigurationUnit[] { configurationUnitNegative, configurationUnitPositive };
TestConfigurationProcessorFactory factory = new TestConfigurationProcessorFactory();
- TestConfigurationSetGroupProcessor setProcessor = factory.CreateTestGroupProcessor(configurationSet);
- setProcessor.ShouldWaitOnAsyncEvent = true;
-
ConfigurationProcessor processor = this.CreateConfigurationProcessorWithDiagnostics(factory);
- List<ConfigurationSetChangeData> progressValues = new List<ConfigurationSetChangeData>();
+ ManualResetEvent startProcessing = new ManualResetEvent(false);
+ factory.CreateSetProcessorDelegate = (f, c) =>
+ {
+ startProcessing.WaitOne();
+ return f.CreateTestGroupProcessor(c);
+ };
var operation = processor.ApplySetAsync(configurationSet, ApplyConfigurationSetFlags.None);
- operation.Progress = (Windows.Foundation.IAsyncOperationWithProgress<ApplyConfigurationSetResult, ConfigurationSetChangeData> op, ConfigurationSetChangeData unitResult) => { progressValues.Add(unitResult); };
- setProcessor.SignalAsyncEvent();
+ List<ConfigurationSetChangeData> progressValues = new List<ConfigurationSetChangeData>();
+ operation.Progress = (Windows.Foundation.IAsyncOperationWithProgress<ApplyConfigurationSetResult, ConfigurationSetChangeData> op, ConfigurationSetChangeData unitResult) => { progressValues.Add(unitResult); };
+ startProcessing.Set();
operation.AsTask().Wait();
ApplyConfigurationSetResult result = operation.GetResults();
@@ -433,7 +437,7 @@ resources:
Assert.Null(result.ResultCode);
Assert.NotNull(result.UnitResults);
Assert.Equal(configurationSet.Units.Count, result.UnitResults.Count);
- Assert.True((configurationSet.Units.Count * 2) + 2 >= progressValues.Count);
+ Assert.Equal((configurationSet.Units.Count * 2) + 2, progressValues.Count);
ApplyConfigurationUnitResult negativeResult = result.UnitResults.First(x => x.Unit == configurationUnitNegative);
@@ -505,16 +509,19 @@ resources:
configurationUnitGroup.Units = new ConfigurationUnit[] { configurationUnitGroupMember };
TestConfigurationProcessorFactory factory = new TestConfigurationProcessorFactory();
- TestConfigurationSetGroupProcessor setProcessor = factory.CreateTestGroupProcessor(configurationSet);
- setProcessor.ShouldWaitOnAsyncEvent = true;
-
ConfigurationProcessor processor = this.CreateConfigurationProcessorWithDiagnostics(factory);
- List<ConfigurationSetChangeData> progressValues = new List<ConfigurationSetChangeData>();
+ ManualResetEvent startProcessing = new ManualResetEvent(false);
+ factory.CreateSetProcessorDelegate = (f, c) =>
+ {
+ startProcessing.WaitOne();
+ return f.CreateTestGroupProcessor(c);
+ };
var operation = processor.ApplySetAsync(configurationSet, ApplyConfigurationSetFlags.None);
- operation.Progress = (Windows.Foundation.IAsyncOperationWithProgress<ApplyConfigurationSetResult, ConfigurationSetChangeData> op, ConfigurationSetChangeData unitResult) => { progressValues.Add(unitResult); };
- setProcessor.SignalAsyncEvent();
+ List<ConfigurationSetChangeData> progressValues = new List<ConfigurationSetChangeData>();
+ operation.Progress = (Windows.Foundation.IAsyncOperationWithProgress<ApplyConfigurationSetResult, ConfigurationSetChangeData> op, ConfigurationSetChangeData unitResult) => { progressValues.Add(unitResult); };
+ startProcessing.Set();
operation.AsTask().Wait();
ApplyConfigurationSetResult result = operation.GetResults();
@@ -522,7 +529,7 @@ resources:
Assert.Null(result.ResultCode);
Assert.NotNull(result.UnitResults);
Assert.Equal(3, result.UnitResults.Count);
- Assert.True(progressValues.Count <= 2 + (3 * 2));
+ Assert.Equal(2 + (3 * 2), progressValues.Count);
foreach (ConfigurationUnit unit in new ConfigurationUnit[] { configurationUnit, configurationUnitGroup, configurationUnitGroupMember })
{
@@ -536,7 +543,7 @@ resources:
Assert.True(unitResult.PreviouslyInDesiredState);
IEnumerable<ConfigurationSetChangeData> unitProgress = progressValues.Where(x => x.Unit == unit);
- Assert.True(unitProgress.Count() <= 2);
+ Assert.Equal(2, unitProgress.Count());
foreach (ConfigurationSetChangeData change in unitProgress)
{
diff --git a/src/Microsoft.Management.Configuration/ConfigurationProcessor.cpp b/src/Microsoft.Management.Configuration/ConfigurationProcessor.cpp
@@ -499,12 +499,9 @@ namespace winrt::Microsoft::Management::Configuration::implementation
}
CATCH_LOG();
- auto applyOperation = groupProcessor.ApplyGroupSettingsAsync();
-
// Forward unit result progress to caller
- applyOperation.Progress([&](const auto&, const IApplyGroupMemberSettingsResult& unitResult)
+ auto applyOperation = groupProcessor.ApplyGroupSettingsAsync([&](const auto&, const IApplyGroupMemberSettingsResult& unitResult)
{
- // Update overall result
auto itr = unitResultMap.find(unitResult.Unit().InstanceIdentifier());
if (itr != unitResultMap.end())
{
@@ -597,13 +594,13 @@ namespace winrt::Microsoft::Management::Configuration::implementation
try
{
- auto testOperation = groupProcessor.TestGroupSettingsAsync();
-
// Forward unit result progress to caller
- testOperation.Progress([&](const auto&, const ITestSettingsResult& unitResult)
+ auto testOperation = groupProcessor.TestGroupSettingsAsync([&](const auto&, const ITestSettingsResult& unitResult)
{
auto testResult = make_self<wil::details::module_count_wrapper<implementation::TestConfigurationUnitResult>>();
testResult->Initialize(unitResult);
+
+ result->AppendUnitResult(*testResult);
progress.Progress(*testResult);
});
@@ -612,7 +609,7 @@ namespace winrt::Microsoft::Management::Configuration::implementation
ITestGroupSettingsResult testResult = testOperation.get();
- // Place all results from the processor into our result
+ // Send telemetry for all results
for (const ITestSettingsResult& unitResult : testResult.UnitResults())
{
auto testUnitResult = make_self<wil::details::module_count_wrapper<implementation::TestConfigurationUnitResult>>();
@@ -624,8 +621,6 @@ namespace winrt::Microsoft::Management::Configuration::implementation
ConfigurationUnitIntent::Assert,
TelemetryTraceLogger::TestAction,
testUnitResult->ResultInformation());
-
- result->AppendUnitResult(*testUnitResult);
}
m_threadGlobals.GetTelemetryLogger().LogConfigProcessingSummaryForTest(*winrt::get_self<implementation::ConfigurationSet>(configurationSet), *result);
diff --git a/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.cpp b/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.cpp
@@ -423,9 +423,7 @@ namespace winrt::Microsoft::Management::Configuration::implementation
if (groupProcessor)
{
- auto applyOperation = groupProcessor.ApplyGroupSettingsAsync();
-
- applyOperation.Progress([&](const auto&, const IApplyGroupMemberSettingsResult& unitResult)
+ auto applyOperation = groupProcessor.ApplyGroupSettingsAsync([&](const auto&, const IApplyGroupMemberSettingsResult& unitResult)
{
m_progress.Progress(unitResult);
});
diff --git a/src/Microsoft.Management.Configuration/DefaultSetGroupProcessor.cpp b/src/Microsoft.Management.Configuration/DefaultSetGroupProcessor.cpp
@@ -33,18 +33,16 @@ namespace winrt::Microsoft::Management::Configuration::implementation
return m_set;
}
- Windows::Foundation::IAsyncOperationWithProgress<ITestGroupSettingsResult, ITestSettingsResult> DefaultSetGroupProcessor::TestGroupSettingsAsync()
+ Windows::Foundation::IAsyncOperation<ITestGroupSettingsResult> DefaultSetGroupProcessor::TestGroupSettingsAsync(Windows::Foundation::EventHandler<ITestSettingsResult> progressHandler)
{
auto strongThis = get_strong();
co_await resume_background();
- auto progress = co_await get_progress_token();
auto cancellation = co_await get_cancellation_token();
cancellation.enable_propagation();
auto result = make_self<wil::details::module_count_wrapper<implementation::TestGroupSettingsResult>>();
result->Group(m_set);
- progress.set_result(*result);
try
{
@@ -80,11 +78,9 @@ namespace winrt::Microsoft::Management::Configuration::implementation
if (groupProcessor)
{
- auto testOperation = groupProcessor.TestGroupSettingsAsync();
-
- testOperation.Progress([&](const auto&, const ITestSettingsResult& unitResult)
+ auto testOperation = groupProcessor.TestGroupSettingsAsync([&](const auto&, const ITestSettingsResult& unitResult)
{
- progress(unitResult);
+ progressHandler(nullptr, unitResult);
});
ITestGroupSettingsResult groupResult = co_await testOperation;
@@ -139,7 +135,7 @@ namespace winrt::Microsoft::Management::Configuration::implementation
try
{
- progress(settingsResult);
+ progressHandler(nullptr, settingsResult);
}
CATCH_LOG();
}
@@ -153,12 +149,12 @@ namespace winrt::Microsoft::Management::Configuration::implementation
}
}
- Windows::Foundation::IAsyncOperationWithProgress<IApplyGroupSettingsResult, IApplyGroupMemberSettingsResult> DefaultSetGroupProcessor::ApplyGroupSettingsAsync()
+ Windows::Foundation::IAsyncOperation<IApplyGroupSettingsResult> DefaultSetGroupProcessor::ApplyGroupSettingsAsync(Windows::Foundation::EventHandler<IApplyGroupMemberSettingsResult> progressHandler)
{
auto strongThis = get_strong();
co_await resume_background();
- ConfigurationSetApplyProcessor applyProcessor{ m_set, m_setProcessor, { co_await winrt::get_progress_token(), co_await winrt::get_cancellation_token() } };
+ ConfigurationSetApplyProcessor applyProcessor{ m_set, m_setProcessor, { std::move(progressHandler), co_await winrt::get_cancellation_token() } };
applyProcessor.Process(m_consistencyCheckOnly);
co_return applyProcessor.Result();
diff --git a/src/Microsoft.Management.Configuration/DefaultSetGroupProcessor.h b/src/Microsoft.Management.Configuration/DefaultSetGroupProcessor.h
@@ -17,9 +17,9 @@ namespace winrt::Microsoft::Management::Configuration::implementation
IInspectable Group();
- Windows::Foundation::IAsyncOperationWithProgress<ITestGroupSettingsResult, ITestSettingsResult> TestGroupSettingsAsync();
+ Windows::Foundation::IAsyncOperation<ITestGroupSettingsResult> TestGroupSettingsAsync(Windows::Foundation::EventHandler<ITestSettingsResult> progressHandler);
- Windows::Foundation::IAsyncOperationWithProgress<IApplyGroupSettingsResult, IApplyGroupMemberSettingsResult> ApplyGroupSettingsAsync();
+ Windows::Foundation::IAsyncOperation<IApplyGroupSettingsResult> ApplyGroupSettingsAsync(Windows::Foundation::EventHandler<IApplyGroupMemberSettingsResult> progressHandler);
private:
void ThrowIf(bool cancellation);
diff --git a/src/Microsoft.Management.Configuration/Microsoft.Management.Configuration.idl b/src/Microsoft.Management.Configuration/Microsoft.Management.Configuration.idl
@@ -572,11 +572,11 @@ namespace Microsoft.Management.Configuration
// Determines if the system is already in the state described by the configuration unit group.
// Progress is expected for every descendant unit and finally the group unit itself.
- Windows.Foundation.IAsyncOperationWithProgress<ITestGroupSettingsResult, ITestSettingsResult> TestGroupSettingsAsync();
+ Windows.Foundation.IAsyncOperation<ITestGroupSettingsResult> TestGroupSettingsAsync(Windows.Foundation.EventHandler<ITestSettingsResult> progressHandler);
// Applies the state described in the configuration unit group.
// Progress is expected for every descendant unit and finally the group unit itself.
- Windows.Foundation.IAsyncOperationWithProgress<IApplyGroupSettingsResult, IApplyGroupMemberSettingsResult> ApplyGroupSettingsAsync();
+ Windows.Foundation.IAsyncOperation<IApplyGroupSettingsResult> ApplyGroupSettingsAsync(Windows.Foundation.EventHandler<IApplyGroupMemberSettingsResult> progressHandler);
}
// The level of the diagnostic information.
