From f223b4a663ca26acac437a9fcacc8988fbec0dfb Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 13 Nov 2018 11:58:54 -0800 Subject: [PATCH] Avoid possible tearing in HeartbeatManager.UtcNow (#3092) --- .../Internal/Http2/Http2Connection.cs | 6 ++-- .../Internal/Http2/Http2Stream.cs | 2 +- src/Kestrel.Core/Internal/HttpConnection.cs | 5 ++-- .../Infrastructure/HeartbeatManager.cs | 10 ++++++- .../Internal/Infrastructure/ISystemClock.cs | 13 +++++++- .../Internal/Infrastructure/SystemClock.cs | 20 ++++++++----- .../Internal/Infrastructure/TimeoutControl.cs | 4 +-- .../Kestrel.Core.Tests/TimeoutControlTests.cs | 28 ++++++++--------- .../Http2/Http2TimeoutTests.cs | 30 +++++++++---------- test/shared/MockSystemClock.cs | 4 +++ 10 files changed, 75 insertions(+), 47 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs index a000db38c1..9a043b7036 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Connection.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Connection.cs @@ -1052,8 +1052,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { if (stream.IsDraining) { - stream.DrainExpiration = - _context.ServiceContext.SystemClock.UtcNow + Constants.RequestBodyDrainTimeout; + stream.DrainExpirationTicks = + _context.ServiceContext.SystemClock.UtcNowTicks + Constants.RequestBodyDrainTimeout.Ticks; _drainingStreams.TryAdd(streamId, stream); } @@ -1096,7 +1096,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 { foreach (var stream in _drainingStreams) { - if (now > stream.Value.DrainExpiration) + if (now.Ticks > stream.Value.DrainExpirationTicks) { RemoveDrainingStream(stream.Key); } diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index e29cc4d792..bd89cb4d46 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -24,7 +24,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private readonly StreamInputFlowControl _inputFlowControl; private readonly StreamOutputFlowControl _outputFlowControl; - internal DateTimeOffset DrainExpiration { get; set; } + internal long DrainExpirationTicks { get; set; } private StreamCompletionFlags _completionState; private readonly object _completionLock = new object(); diff --git a/src/Kestrel.Core/Internal/HttpConnection.cs b/src/Kestrel.Core/Internal/HttpConnection.cs index 3b77900ffa..1fdd228fc7 100644 --- a/src/Kestrel.Core/Internal/HttpConnection.cs +++ b/src/Kestrel.Core/Internal/HttpConnection.cs @@ -113,7 +113,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal using (connectionLifetimeNotificationFeature?.ConnectionClosedRequested.Register(state => ((HttpConnection)state).StopProcessingNextRequest(), this)) { // Ensure TimeoutControl._lastTimestamp is initialized before anything that could set timeouts runs. - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNowTicks); _context.ConnectionFeatures.Set(_timeoutControl); @@ -356,7 +356,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal return; } - var now = _systemClock.UtcNow; + // It's safe to use UtcNowUnsynchronized since Tick is called by the Heartbeat. + var now = _systemClock.UtcNowUnsynchronized; _timeoutControl.Tick(now); _requestProcessor?.Tick(now); } diff --git a/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs b/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs index bc54e73385..97cfee5272 100644 --- a/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs +++ b/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Threading; namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure { @@ -10,6 +11,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure private readonly ConnectionManager _connectionManager; private readonly Action _walkCallback; private DateTimeOffset _now; + private long _nowTicks; public HeartbeatManager(ConnectionManager connectionManager) { @@ -17,11 +19,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure _walkCallback = WalkCallback; } - public DateTimeOffset UtcNow => _now; + public DateTimeOffset UtcNow => new DateTimeOffset(UtcNowTicks, TimeSpan.Zero); + + public long UtcNowTicks => Volatile.Read(ref _nowTicks); + + public DateTimeOffset UtcNowUnsynchronized => _now; public void OnHeartbeat(DateTimeOffset now) { _now = now; + Volatile.Write(ref _nowTicks, now.Ticks); + _connectionManager.Walk(_walkCallback); } diff --git a/src/Kestrel.Core/Internal/Infrastructure/ISystemClock.cs b/src/Kestrel.Core/Internal/Infrastructure/ISystemClock.cs index ddc5b1fd66..7b64f79d36 100644 --- a/src/Kestrel.Core/Internal/Infrastructure/ISystemClock.cs +++ b/src/Kestrel.Core/Internal/Infrastructure/ISystemClock.cs @@ -11,8 +11,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure public interface ISystemClock { /// - /// Retrieves the current system time in UTC. + /// Retrieves the current UTC system time. /// DateTimeOffset UtcNow { get; } + + /// + /// Retrieves ticks for the current UTC system time. + /// + long UtcNowTicks { get; } + + /// + /// Retrieves the current UTC system time. + /// This is only safe to use from code called by the . + /// + DateTimeOffset UtcNowUnsynchronized { get; } } } diff --git a/src/Kestrel.Core/Internal/Infrastructure/SystemClock.cs b/src/Kestrel.Core/Internal/Infrastructure/SystemClock.cs index 1284ef9f4f..be903d6ba5 100644 --- a/src/Kestrel.Core/Internal/Infrastructure/SystemClock.cs +++ b/src/Kestrel.Core/Internal/Infrastructure/SystemClock.cs @@ -11,14 +11,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure internal class SystemClock : ISystemClock { /// - /// Retrieves the current system time in UTC. + /// Retrieves the current UTC system time. /// - public DateTimeOffset UtcNow - { - get - { - return DateTimeOffset.UtcNow; - } - } + public DateTimeOffset UtcNow => DateTimeOffset.UtcNow; + + /// + /// Retrieves ticks for the current UTC system time. + /// + public long UtcNowTicks => DateTimeOffset.UtcNow.Ticks; + + /// + /// Retrieves the current UTC system time. + /// + public DateTimeOffset UtcNowUnsynchronized => DateTimeOffset.UtcNow; } } diff --git a/src/Kestrel.Core/Internal/Infrastructure/TimeoutControl.cs b/src/Kestrel.Core/Internal/Infrastructure/TimeoutControl.cs index af1aff9cc1..61ceb814d9 100644 --- a/src/Kestrel.Core/Internal/Infrastructure/TimeoutControl.cs +++ b/src/Kestrel.Core/Internal/Infrastructure/TimeoutControl.cs @@ -40,9 +40,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure internal IDebugger Debugger { get; set; } = DebuggerWrapper.Singleton; - public void Initialize(DateTimeOffset now) + internal void Initialize(long nowTicks) { - _lastTimestamp = now.Ticks; + _lastTimestamp = nowTicks; } public void Tick(DateTimeOffset now) diff --git a/test/Kestrel.Core.Tests/TimeoutControlTests.cs b/test/Kestrel.Core.Tests/TimeoutControlTests.cs index d23f8156de..170d7b1479 100644 --- a/test/Kestrel.Core.Tests/TimeoutControlTests.cs +++ b/test/Kestrel.Core.Tests/TimeoutControlTests.cs @@ -31,7 +31,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests _timeoutControl.Debugger = mockDebugger.Object; var now = DateTimeOffset.Now; - _timeoutControl.Initialize(now); + _timeoutControl.Initialize(now.Ticks); _timeoutControl.SetTimeout(1, TimeoutReason.RequestHeaders); _timeoutControl.Tick(now.AddTicks(2).Add(Heartbeat.Interval)); @@ -67,7 +67,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Initialize timestamp var now = DateTimeOffset.UtcNow; - _timeoutControl.Initialize(now); + _timeoutControl.Initialize(now.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); @@ -99,7 +99,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Initialize timestamp var now = DateTimeOffset.UtcNow; - _timeoutControl.Initialize(now); + _timeoutControl.Initialize(now.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); @@ -159,7 +159,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var minRate = new MinDataRate(bytesPerSecond: 100, gracePeriod: TimeSpan.FromSeconds(2)); // Initialize timestamp - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNow.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); @@ -215,7 +215,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var minRate = new MinDataRate(bytesPerSecond: 100, gracePeriod: TimeSpan.FromSeconds(2)); // Initialize timestamp - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNow.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); @@ -263,7 +263,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var startTime = _systemClock.UtcNow; // Initialize timestamp - _timeoutControl.Initialize(startTime); + _timeoutControl.Initialize(startTime.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); @@ -295,7 +295,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Initialize timestamp var now = DateTimeOffset.UtcNow; - _timeoutControl.Initialize(now); + _timeoutControl.Initialize(now.Ticks); _timeoutControl.InitializeHttp2(flowControl); _timeoutControl.StartRequestBody(minRate); @@ -341,7 +341,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Initialize timestamp var now = DateTimeOffset.UtcNow; - _timeoutControl.Initialize(now); + _timeoutControl.Initialize(now.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); @@ -379,7 +379,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var minRate = new MinDataRate(bytesPerSecond: 100, gracePeriod: TimeSpan.FromSeconds(2)); // Initialize timestamp - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNow.Ticks); // Should complete within 4 seconds, but the timeout is adjusted by adding Heartbeat.Interval _timeoutControl.BytesWrittenToBuffer(minRate, 400); @@ -398,7 +398,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Initialize timestamp var startTime = _systemClock.UtcNow; - _timeoutControl.Initialize(startTime); + _timeoutControl.Initialize(startTime.Ticks); // Should complete within 1 second, but the timeout is adjusted by adding Heartbeat.Interval _timeoutControl.BytesWrittenToBuffer(minRate, 100); @@ -422,7 +422,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var minRate = new MinDataRate(bytesPerSecond: 100, gracePeriod: TimeSpan.FromSeconds(2)); // Initialize timestamp - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNow.Ticks); // Should complete within 5 seconds, but the timeout is adjusted by adding Heartbeat.Interval _timeoutControl.BytesWrittenToBuffer(minRate, 500); @@ -456,7 +456,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Initialize timestamp var startTime = _systemClock.UtcNow; - _timeoutControl.Initialize(startTime); + _timeoutControl.Initialize(startTime.Ticks); // 5 consecutive 100 byte writes. for (var i = 0; i < numWrites - 1; i++) @@ -487,7 +487,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var minRate = new MinDataRate(bytesPerSecond: 100, gracePeriod: TimeSpan.FromSeconds(2)); // Initialize timestamp - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNow.Ticks); // Should complete within 4 seconds, but the timeout is adjusted by adding Heartbeat.Interval _timeoutControl.BytesWrittenToBuffer(minRate, 400); @@ -512,7 +512,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var minRate = new MinDataRate(bytesPerSecond, gracePeriod); // Initialize timestamp - _timeoutControl.Initialize(_systemClock.UtcNow); + _timeoutControl.Initialize(_systemClock.UtcNow.Ticks); _timeoutControl.StartRequestBody(minRate); _timeoutControl.StartTimingRead(); diff --git a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs index 42e232cbbd..0277306a38 100644 --- a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs @@ -25,7 +25,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var mockSystemClock = _serviceContext.MockSystemClock; var limits = _serviceContext.ServerOptions.Limits; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); CreateConnection(); @@ -50,7 +50,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var mockSystemClock = _serviceContext.MockSystemClock; var limits = _serviceContext.ServerOptions.Limits; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_noopApplication); @@ -73,7 +73,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var mockSystemClock = _serviceContext.MockSystemClock; var limits = _serviceContext.ServerOptions.Limits; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_noopApplication); @@ -128,7 +128,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var mockSystemClock = _serviceContext.MockSystemClock; var limits = _serviceContext.ServerOptions.Limits;; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_noopApplication); @@ -165,7 +165,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests var mockSystemClock = _serviceContext.MockSystemClock; var limits = _serviceContext.ServerOptions.Limits; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_noopApplication); @@ -272,7 +272,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Disable response buffering so "socket" backpressure is observed immediately. limits.MaxResponseBufferSize = 0; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -328,7 +328,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Disable response buffering so "socket" backpressure is observed immediately. limits.MaxResponseBufferSize = 0; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -386,7 +386,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // This only affects the stream windows. The connection-level window is always initialized at 64KiB. _clientSettings.InitialWindowSize = 6; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -440,7 +440,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // This only affects the stream windows. The connection-level window is always initialized at 64KiB. _clientSettings.InitialWindowSize = (uint)_maxData.Length - 1; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -496,7 +496,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // This only affects the stream windows. The connection-level window is always initialized at 64KiB. _clientSettings.InitialWindowSize = (uint)_maxData.Length - 1; - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -561,7 +561,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Use non-default value to ensure the min request and response rates aren't mixed up. limits.MinRequestBodyDataRate = new MinDataRate(480, TimeSpan.FromSeconds(2.5)); - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -610,7 +610,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Use non-default value to ensure the min request and response rates aren't mixed up. limits.MinRequestBodyDataRate = new MinDataRate(480, TimeSpan.FromSeconds(2.5)); - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -663,7 +663,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Use non-default value to ensure the min request and response rates aren't mixed up. limits.MinRequestBodyDataRate = new MinDataRate(480, TimeSpan.FromSeconds(2.5)); - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -732,7 +732,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Use non-default value to ensure the min request and response rates aren't mixed up. limits.MinRequestBodyDataRate = new MinDataRate(480, TimeSpan.FromSeconds(2.5)); - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(_echoApplication); @@ -807,7 +807,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests // Use non-default value to ensure the min request and response rates aren't mixed up. limits.MinRequestBodyDataRate = new MinDataRate(480, TimeSpan.FromSeconds(2.5)); - _timeoutControl.Initialize(mockSystemClock.UtcNow); + _timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks); await InitializeConnectionAsync(async context => { diff --git a/test/shared/MockSystemClock.cs b/test/shared/MockSystemClock.cs index 3977653630..5ec904aad0 100644 --- a/test/shared/MockSystemClock.cs +++ b/test/shared/MockSystemClock.cs @@ -24,6 +24,10 @@ namespace Microsoft.AspNetCore.Testing } } + public long UtcNowTicks => UtcNow.Ticks; + + public DateTimeOffset UtcNowUnsynchronized => UtcNow; + public int UtcNowCalled { get; private set; } } }