Skip to content
This repository was archived by the owner on Dec 8, 2018. It is now read-only.

Commit e0f1710

Browse files
rynowakrynowak
committed
PR feedback
1 parent 3402e2a commit e0f1710

File tree

5 files changed

+81
-58
lines changed

5 files changed

+81
-58
lines changed

‎samples/HealthChecksSample/BasicStartup.cs

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@
22
using Microsoft.AspNetCore.Hosting;
33
using Microsoft.AspNetCore.Http;
44
using Microsoft.Extensions.DependencyInjection;
5-
using Microsoft.Extensions.Diagnostics.HealthChecks;
6-
using Microsoft.Extensions.Hosting;
75

86
namespace HealthChecksSample
97
{
@@ -16,7 +14,7 @@ public void ConfigureServices(IServiceCollection services)
1614
services.AddHealthChecks();
1715
}
1816

19-
public void Configure(IApplicationBuilder app, Microsoft.AspNetCore.Hosting.IHostingEnvironment env)
17+
public void Configure(IApplicationBuilder app, IHostingEnvironment env)
2018
{
2119
// This will register the health checks middleware at the URL /health.
2220
//

‎samples/WelcomePageSample/web.config

Lines changed: 0 additions & 9 deletions
This file was deleted.

‎src/Microsoft.Extensions.Diagnostics.HealthChecks/HealthCheckPublisherHostedService.cs

Lines changed: 16 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,11 @@ public HealthCheckPublisherHostedService(
5353
_options = options;
5454
_logger = logger;
5555
_publishers = publishers.ToArray();
56+
57+
_stopping = new CancellationTokenSource();
5658
}
5759

58-
internal bool IsCancelled => _stopping == null || _stopping.IsCancellationRequested;
60+
internal bool IsStopping => _stopping.IsCancellationRequested;
5961

6062
internal bool IsTimerRunning => _timer != null;
6163

@@ -66,8 +68,6 @@ public Task StartAsync(CancellationToken cancellationToken = default)
6668
return Task.CompletedTask;
6769
}
6870

69-
_stopping = new CancellationTokenSource();
70-
7171
// IMPORTANT - make sure this is the last thing that happens in this method. The timer can
7272
// fire before other code runs.
7373
_timer = NonCapturingTimer.Create(Timer_Tick, null, dueTime: _options.Value.Delay, period: _options.Value.Period);
@@ -77,6 +77,15 @@ public Task StartAsync(CancellationToken cancellationToken = default)
7777

7878
public Task StopAsync(CancellationToken cancellationToken = default)
7979
{
80+
try
81+
{
82+
_stopping.Cancel();
83+
}
84+
catch
85+
{
86+
// Ignore exceptions thrown as a result of a cancellation.
87+
}
88+
8089
if (_publishers.Length == 0)
8190
{
8291
return Task.CompletedTask;
@@ -85,9 +94,6 @@ public Task StopAsync(CancellationToken cancellationToken = default)
8594
_timer?.Dispose();
8695
_timer = null;
8796

88-
_stopping?.Cancel();
89-
_stopping?.Dispose();
90-
_stopping = null;
9197

9298
return Task.CompletedTask;
9399
}
@@ -110,16 +116,13 @@ internal async Task RunAsync()
110116
var timeout = _options.Value.Timeout;
111117

112118
cancellation = CancellationTokenSource.CreateLinkedTokenSource(_stopping.Token);
113-
if (timeout != Timeout.InfiniteTimeSpan)
114-
{
115-
cancellation.CancelAfter(timeout);
116-
}
119+
cancellation.CancelAfter(timeout);
117120

118121
await RunAsyncCore(cancellation.Token);
119122

120123
Logger.HealthCheckPublisherProcessingEnd(_logger, duration.GetElapsedTime());
121124
}
122-
catch (OperationCanceledException) when (IsCancelled)
125+
catch (OperationCanceledException) when (IsStopping)
123126
{
124127
// This is a cancellation - if the app is shutting down we want to ignore it. Otherwise, it's
125128
// a timeout and we want to log it.
@@ -147,17 +150,7 @@ private async Task RunAsyncCore(CancellationToken cancellationToken)
147150
var tasks = new Task[publishers.Length];
148151
for (var i = 0; i < publishers.Length; i++)
149152
{
150-
try
151-
{
152-
tasks[i] = RunPublisherAsync(publishers[i], report, cancellationToken);
153-
}
154-
catch (Exception ex)
155-
{
156-
// Handle any exceptions that are thrown synchronously - this allows us to start
157-
// all publishers before awaiting any of them. We don't want a single slow publisher
158-
// to prevent others from getting started.
159-
tasks[i] = Task.FromException(ex);
160-
}
153+
tasks[i] = RunPublisherAsync(publishers[i], report, cancellationToken);
161154
}
162155

163156
await Task.WhenAll(tasks);
@@ -174,7 +167,7 @@ private async Task RunPublisherAsync(IHealthCheckPublisher publisher, HealthRepo
174167
await publisher.PublishAsync(report, cancellationToken);
175168
Logger.HealthCheckPublisherEnd(_logger, publisher, duration.GetElapsedTime());
176169
}
177-
catch (OperationCanceledException) when (IsCancelled)
170+
catch (OperationCanceledException) when (IsStopping)
178171
{
179172
// This is a cancellation - if the app is shutting down we want to ignore it. Otherwise, it's
180173
// a timeout and we want to log it.

‎src/Microsoft.Extensions.Diagnostics.HealthChecks/HealthCheckPublisherOptions.cs

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,26 +10,67 @@ namespace Microsoft.Extensions.Diagnostics.HealthChecks
1010
/// </summary>
1111
public sealed class HealthCheckPublisherOptions
1212
{
13+
private TimeSpan _delay;
14+
private TimeSpan _period;
15+
16+
public HealthCheckPublisherOptions()
17+
{
18+
_delay = TimeSpan.FromSeconds(5);
19+
_period = TimeSpan.FromSeconds(30);
20+
}
21+
1322
/// <summary>
1423
/// Gets or sets the initial delay applied after the application starts before executing
1524
/// <see cref="IHealthCheckPublisher"/> instances. The delay is applied once at startup, and does
1625
/// not apply to subsequent iterations. The default value is 5 seconds.
1726
/// </summary>
18-
public TimeSpan Delay { get; set; } = TimeSpan.FromSeconds(5);
27+
public TimeSpan Delay
28+
{
29+
get => _delay;
30+
set
31+
{
32+
if (value == System.Threading.Timeout.InfiniteTimeSpan)
33+
{
34+
throw new ArgumentException($"The {nameof(Delay)} must not be infinite.", nameof(value));
35+
}
36+
37+
_delay = value;
38+
}
39+
}
1940

2041
/// <summary>
2142
/// Gets or sets the period of <see cref="IHealthCheckPublisher"/> execution. The default value is
2243
/// 30 seconds.
2344
/// </summary>
24-
public TimeSpan Period { get; set; } = TimeSpan.FromSeconds(30);
45+
/// <remarks>
46+
/// The <see cref="Period"/> cannot be set to a value lower than 1 second.
47+
/// </remarks>
48+
public TimeSpan Period
49+
{
50+
get => _period;
51+
set
52+
{
53+
if (value < TimeSpan.FromSeconds(1))
54+
{
55+
throw new ArgumentException($"The {nameof(Period)} must be greater than or equal to one second.", nameof(value));
56+
}
57+
58+
if (value == System.Threading.Timeout.InfiniteTimeSpan)
59+
{
60+
throw new ArgumentException($"The {nameof(Period)} must not be infinite.", nameof(value));
61+
}
62+
63+
_delay = value;
64+
}
65+
}
2566

2667
/// <summary>
2768
/// Gets or sets a predicate that is used to filter the set of health checks executed.
2869
/// </summary>
2970
/// <remarks>
3071
/// If <see cref="Predicate"/> is <c>null</c>, the health check publisher service will run all
3172
/// registered health checks - this is the default behavior. To run a subset of health checks,
32-
/// provide a function that filters the set of checks.
73+
/// provide a function that filters the set of checks. The predicate will be evaluated each period.
3374
/// </remarks>
3475
public Func<HealthCheckRegistration, bool> Predicate { get; set; }
3576

‎test/Microsoft.Extensions.Diagnostics.HealthChecks.Tests/HealthCheckPublisherHostedServiceTest.cs

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,13 @@ public async Task StartAsync_WithoutPublishers_DoesNotStartTimer()
3434

3535
// Assert
3636
Assert.False(service.IsTimerRunning);
37-
Assert.True(service.IsCancelled);
37+
Assert.False(service.IsStopping);
3838
}
3939
finally
4040
{
4141
await service.StopAsync();
4242
Assert.False(service.IsTimerRunning);
43-
Assert.True(service.IsCancelled);
43+
Assert.True(service.IsStopping);
4444
}
4545
}
4646

@@ -62,13 +62,13 @@ public async Task StartAsync_WithPublishers_StartsTimer()
6262

6363
// Assert
6464
Assert.True(service.IsTimerRunning);
65-
Assert.False(service.IsCancelled);
65+
Assert.False(service.IsStopping);
6666
}
6767
finally
6868
{
6969
await service.StopAsync();
7070
Assert.False(service.IsTimerRunning);
71-
Assert.True(service.IsCancelled);
71+
Assert.True(service.IsStopping);
7272
}
7373
}
7474

@@ -107,13 +107,13 @@ public async Task StartAsync_WithPublishers_StartsTimer_RunsPublishers()
107107

108108
// Assert
109109
Assert.True(service.IsTimerRunning);
110-
Assert.False(service.IsCancelled);
110+
Assert.False(service.IsStopping);
111111
}
112112
finally
113113
{
114114
await service.StopAsync();
115115
Assert.False(service.IsTimerRunning);
116-
Assert.True(service.IsCancelled);
116+
Assert.True(service.IsStopping);
117117
}
118118
}
119119

@@ -147,7 +147,7 @@ public async Task StopAsync_CancelsExecution()
147147
// Assert
148148
await AssertCancelledAsync(publishers[0].Entries[0].cancellationToken);
149149
Assert.False(service.IsTimerRunning);
150-
Assert.True(service.IsCancelled);
150+
Assert.True(service.IsStopping);
151151

152152
unblock.SetResult(null);
153153

@@ -157,7 +157,7 @@ public async Task StopAsync_CancelsExecution()
157157
{
158158
await service.StopAsync();
159159
Assert.False(service.IsTimerRunning);
160-
Assert.True(service.IsCancelled);
160+
Assert.True(service.IsStopping);
161161
}
162162
}
163163

@@ -191,7 +191,7 @@ public async Task RunAsync_WaitsForCompletion_Single()
191191

192192
// Assert
193193
Assert.True(service.IsTimerRunning);
194-
Assert.False(service.IsCancelled);
194+
Assert.False(service.IsStopping);
195195

196196
for (var i = 0; i < publishers.Length; i++)
197197
{
@@ -203,7 +203,7 @@ public async Task RunAsync_WaitsForCompletion_Single()
203203
{
204204
await service.StopAsync();
205205
Assert.False(service.IsTimerRunning);
206-
Assert.True(service.IsCancelled);
206+
Assert.True(service.IsStopping);
207207
}
208208

209209
Assert.Collection(
@@ -257,7 +257,7 @@ public async Task RunAsync_WaitsForCompletion_Multiple()
257257

258258
// Assert
259259
Assert.True(service.IsTimerRunning);
260-
Assert.False(service.IsCancelled);
260+
Assert.False(service.IsStopping);
261261

262262
for (var i = 0; i < publishers.Length; i++)
263263
{
@@ -269,7 +269,7 @@ public async Task RunAsync_WaitsForCompletion_Multiple()
269269
{
270270
await service.StopAsync();
271271
Assert.False(service.IsTimerRunning);
272-
Assert.True(service.IsCancelled);
272+
Assert.True(service.IsStopping);
273273
}
274274
}
275275

@@ -307,13 +307,13 @@ public async Task RunAsync_PublishersCanTimeout()
307307

308308
// Assert
309309
Assert.True(service.IsTimerRunning);
310-
Assert.False(service.IsCancelled);
310+
Assert.False(service.IsStopping);
311311
}
312312
finally
313313
{
314314
await service.StopAsync();
315315
Assert.False(service.IsTimerRunning);
316-
Assert.True(service.IsCancelled);
316+
Assert.True(service.IsStopping);
317317
}
318318

319319
Assert.Collection(
@@ -363,7 +363,7 @@ public async Task RunAsync_CanFilterHealthChecks()
363363
{
364364
await service.StopAsync();
365365
Assert.False(service.IsTimerRunning);
366-
Assert.True(service.IsCancelled);
366+
Assert.True(service.IsStopping);
367367
}
368368
}
369369

@@ -391,7 +391,7 @@ public async Task RunAsync_HandlesExceptions()
391391
{
392392
await service.StopAsync();
393393
Assert.False(service.IsTimerRunning);
394-
Assert.True(service.IsCancelled);
394+
Assert.True(service.IsStopping);
395395
}
396396

397397
Assert.Collection(
@@ -435,7 +435,7 @@ public async Task RunAsync_HandlesExceptions_Multiple()
435435
{
436436
await service.StopAsync();
437437
Assert.False(service.IsTimerRunning);
438-
Assert.True(service.IsCancelled);
438+
Assert.True(service.IsStopping);
439439
}
440440
}
441441

@@ -455,9 +455,9 @@ private HealthCheckPublisherHostedService CreateService(
455455
// All of the tests that rely on the timer will set their own values for speed.
456456
serviceCollection.Configure<HealthCheckPublisherOptions>(options =>
457457
{
458-
options.Delay = Timeout.InfiniteTimeSpan;
459-
options.Period = Timeout.InfiniteTimeSpan;
460-
options.Timeout = Timeout.InfiniteTimeSpan;
458+
options.Delay = TimeSpan.FromMinutes(5);
459+
options.Period = TimeSpan.FromMinutes(5);
460+
options.Timeout = TimeSpan.FromMinutes(5);
461461
});
462462

463463
if (publishers != null)

0 commit comments

Comments
 (0)