Take in account code review

\n\nCommit migrated from ecea8c802e

src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs

@@ -39,75 +39,32 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
             Func<HealthCheckRegistration, bool> predicate,
             CancellationToken cancellationToken = default)
         {
-            async Task<(string registrationName, HealthReportEntry result)> RunCheckAsync(IServiceScope scope, HealthCheckRegistration registration)
+            var registrations = _options.Value.Registrations;
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-
-                var healthCheck = registration.Factory(scope.ServiceProvider);
-
-                // If the health check does things like make Database queries using EF or backend HTTP calls,
-                // it may be valuable to know that logs it generates are part of a health check. So we start a scope.
-                using (_logger.BeginScope(new HealthCheckLogScope(registration.Name)))
-                {
-                    var stopwatch = ValueStopwatch.StartNew();
-                    var context = new HealthCheckContext { Registration = registration };
-
-                    Log.HealthCheckBegin(_logger, registration);
-
-                    HealthReportEntry entry;
-                    try
-                    {
-                        var result = await healthCheck.CheckHealthAsync(context, cancellationToken);
-                        var duration = stopwatch.GetElapsedTime();
-
-                        entry = new HealthReportEntry(
-                            status: result.Status,
-                            description: result.Description,
-                            duration: duration,
-                            exception: result.Exception,
-                            data: result.Data);
-
-                        Log.HealthCheckEnd(_logger, registration, entry, duration);
-                        Log.HealthCheckData(_logger, registration, entry);
-                    }
-
-                    // Allow cancellation to propagate.
-                    catch (Exception ex) when (ex as OperationCanceledException == null)
-                    {
-                        var duration = stopwatch.GetElapsedTime();
-                        entry = new HealthReportEntry(
-                            status: HealthStatus.Unhealthy,
-                            description: ex.Message,
-                            duration: duration,
-                            exception: ex,
-                            data: null);
-
-                        Log.HealthCheckError(_logger, registration, ex, duration);
-                    }
-
-                    return (registration.Name, entry);
-                }
-            }
-
-            IEnumerable<HealthCheckRegistration> registrations = _options.Value.Registrations;
             if (predicate != null)
             {
-                registrations = registrations.Where(predicate);
+                registrations = registrations.Where(predicate).ToArray();
             }
 
             var totalTime = ValueStopwatch.StartNew();
             Log.HealthCheckProcessingBegin(_logger);
 
-            (string registrationName, HealthReportEntry result)[] results;
+            var tasks = new Task<HealthReportEntry>[registrations.Count];
+            var index = 0;
             using (var scope = _scopeFactory.CreateScope())
             {
-                results = await Task.WhenAll(registrations.Select(r => RunCheckAsync(scope, r)));
+                foreach (var registration in registrations)
+                {
+                    tasks[index++] = RunCheckAsync(scope, registration, cancellationToken);
+                }
+
+                await Task.WhenAll(tasks).ConfigureAwait(false);
             }
 
+            index = 0;
             var entries = new Dictionary<string, HealthReportEntry>(StringComparer.OrdinalIgnoreCase);
-            foreach (var (registrationName, result) in results)
+            foreach (var registration in registrations)
             {
-                entries[registrationName] = result;
+                entries[registration.Name] = tasks[index++].Result;
             }
 
             var totalElapsedTime = totalTime.GetElapsedTime();
@@ -116,6 +73,58 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
             return report;
         }
 
+        private async Task<HealthReportEntry> RunCheckAsync(IServiceScope scope, HealthCheckRegistration registration, CancellationToken cancellationToken)
+        {
+            await Task.Yield();
+
+            cancellationToken.ThrowIfCancellationRequested();
+
+            var healthCheck = registration.Factory(scope.ServiceProvider);
+
+            // If the health check does things like make Database queries using EF or backend HTTP calls,
+            // it may be valuable to know that logs it generates are part of a health check. So we start a scope.
+            using (_logger.BeginScope(new HealthCheckLogScope(registration.Name)))
+            {
+                var stopwatch = ValueStopwatch.StartNew();
+                var context = new HealthCheckContext { Registration = registration };
+
+                Log.HealthCheckBegin(_logger, registration);
+
+                HealthReportEntry entry;
+                try
+                {
+                    var result = await healthCheck.CheckHealthAsync(context, cancellationToken);
+                    var duration = stopwatch.GetElapsedTime();
+
+                    entry = new HealthReportEntry(
+                        status: result.Status,
+                        description: result.Description,
+                        duration: duration,
+                        exception: result.Exception,
+                        data: result.Data);
+
+                    Log.HealthCheckEnd(_logger, registration, entry, duration);
+                    Log.HealthCheckData(_logger, registration, entry);
+                }
+
+                // Allow cancellation to propagate.
+                catch (Exception ex) when (ex as OperationCanceledException == null)
+                {
+                    var duration = stopwatch.GetElapsedTime();
+                    entry = new HealthReportEntry(
+                        status: HealthStatus.Unhealthy,
+                        description: ex.Message,
+                        duration: duration,
+                        exception: ex,
+                        data: null);
+
+                    Log.HealthCheckError(_logger, registration, ex, duration);
+                }
+
+                return entry;
+            }
+        }
+
         private static void ValidateRegistrations(IEnumerable<HealthCheckRegistration> registrations)
         {
             // Scan the list for duplicate names to provide a better error if there are duplicates.

src/HealthChecks/HealthChecks/test/DefaultHealthCheckServiceTest.cs

@@ -375,12 +375,50 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
                 });
         }
 
-        private static DefaultHealthCheckService CreateHealthChecksService(Action<IHealthChecksBuilder> configure)
+        [Fact]
+        public async Task CheckHealthAsync_ChecksAreRunInParallel()
+        {
+            // Arrange
+            var sink = new TestSink();
+            async Task<HealthCheckResult> CheckMethod()
+            {
+                await Task.Delay(100);
+                return HealthCheckResult.Healthy();
+            }
+            var service = CreateHealthChecksService(b =>
+            {
+                b.AddAsyncCheck("test1", CheckMethod);
+                b.AddAsyncCheck("test2", CheckMethod);
+                b.AddAsyncCheck("test3", CheckMethod);
+            }, sink);
+
+            // Act
+            _ = await service.CheckHealthAsync();
+
+            // Assert
+            Assert.Collection(
+                sink.Writes,
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingBegin, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
+                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingEnd, entry.EventId); });
+        }
+
+        private static DefaultHealthCheckService CreateHealthChecksService(Action<IHealthChecksBuilder> configure, ITestSink sink = null)
         {
             var services = new ServiceCollection();
             services.AddLogging();
             services.AddOptions();
 
+            if (sink != null)
+            {
+                services.AddSingleton<ILoggerFactory>(new TestLoggerFactory(sink, enabled: true));
+            }
+
             var builder = services.AddHealthChecks();
             if (configure != null)
             {

src/HealthChecks/HealthChecks/test/HealthCheckPublisherHostedServiceTest.cs

@@ -211,8 +211,8 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
                 entry => { Assert.Equal(HealthCheckPublisherHostedService.EventIds.HealthCheckPublisherProcessingBegin, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingBegin, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
-                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
+                entry => { Assert.Contains(entry.EventId, new[] { DefaultHealthCheckService.EventIds.HealthCheckBegin, DefaultHealthCheckService.EventIds.HealthCheckEnd }); },
-                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
+                entry => { Assert.Contains(entry.EventId, new[] { DefaultHealthCheckService.EventIds.HealthCheckBegin, DefaultHealthCheckService.EventIds.HealthCheckEnd }); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingEnd, entry.EventId); }, 
                 entry => { Assert.Equal(HealthCheckPublisherHostedService.EventIds.HealthCheckPublisherBegin, entry.EventId); },
@@ -321,8 +321,8 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
                 entry => { Assert.Equal(HealthCheckPublisherHostedService.EventIds.HealthCheckPublisherProcessingBegin, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingBegin, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
-                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
+                entry => { Assert.Contains(entry.EventId, new[] { DefaultHealthCheckService.EventIds.HealthCheckBegin, DefaultHealthCheckService.EventIds.HealthCheckEnd }); },
-                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
+                entry => { Assert.Contains(entry.EventId, new[] { DefaultHealthCheckService.EventIds.HealthCheckBegin, DefaultHealthCheckService.EventIds.HealthCheckEnd }); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingEnd, entry.EventId); },
                 entry => { Assert.Equal(HealthCheckPublisherHostedService.EventIds.HealthCheckPublisherBegin, entry.EventId); },
@@ -399,8 +399,8 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
                 entry => { Assert.Equal(HealthCheckPublisherHostedService.EventIds.HealthCheckPublisherProcessingBegin, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingBegin, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
-                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
+                entry => { Assert.Contains(entry.EventId, new[] { DefaultHealthCheckService.EventIds.HealthCheckBegin, DefaultHealthCheckService.EventIds.HealthCheckEnd }); },
-                entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckBegin, entry.EventId); },
+                entry => { Assert.Contains(entry.EventId, new[] { DefaultHealthCheckService.EventIds.HealthCheckBegin, DefaultHealthCheckService.EventIds.HealthCheckEnd }); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckEnd, entry.EventId); },
                 entry => { Assert.Equal(DefaultHealthCheckService.EventIds.HealthCheckProcessingEnd, entry.EventId); },
                 entry => { Assert.Equal(HealthCheckPublisherHostedService.EventIds.HealthCheckPublisherBegin, entry.EventId); },