From f70ba53253480a0c9b672537b8b13058b3e16160 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 27 Aug 2018 10:59:29 -0700 Subject: [PATCH] Fix race when SystemClock is accessed before first heartbeat (#2851) --- .../Internal/Http/DateHeaderValueManager.cs | 6 +++--- .../Internal/Infrastructure/Heartbeat.cs | 9 ++------- .../Internal/Infrastructure/HeartbeatManager.cs | 3 ++- src/Kestrel.Core/KestrelServer.cs | 5 +++-- .../DateHeaderValueManagerTests.cs | 16 ++++++---------- .../HttpsTests.cs | 2 +- .../KeepAliveTimeoutTests.cs | 12 ++++++------ .../RequestBodyTimeoutTests.cs | 4 ++-- .../RequestHeadersTimeoutTests.cs | 8 ++++---- .../RequestTests.cs | 2 +- .../RequestTests.cs | 4 +++- test/shared/TestServiceContext.cs | 15 ++++++++------- 12 files changed, 41 insertions(+), 45 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http/DateHeaderValueManager.cs b/src/Kestrel.Core/Internal/Http/DateHeaderValueManager.cs index b2cb8743640c..ee82fd1bc1c9 100644 --- a/src/Kestrel.Core/Internal/Http/DateHeaderValueManager.cs +++ b/src/Kestrel.Core/Internal/Http/DateHeaderValueManager.cs @@ -22,14 +22,14 @@ public class DateHeaderValueManager : IHeartbeatHandler /// Initializes a new instance of the class. /// public DateHeaderValueManager() - : this(systemClock: new SystemClock()) + : this(DateTimeOffset.UtcNow) { } // Internal for testing - internal DateHeaderValueManager(ISystemClock systemClock) + internal DateHeaderValueManager(DateTimeOffset initialUtcNow) { - SetDateValues(systemClock.UtcNow); + SetDateValues(initialUtcNow); } /// diff --git a/src/Kestrel.Core/Internal/Infrastructure/Heartbeat.cs b/src/Kestrel.Core/Internal/Infrastructure/Heartbeat.cs index fb0f17d83b84..5937002304c9 100644 --- a/src/Kestrel.Core/Internal/Infrastructure/Heartbeat.cs +++ b/src/Kestrel.Core/Internal/Infrastructure/Heartbeat.cs @@ -19,18 +19,13 @@ public class Heartbeat : IDisposable private Timer _timer; private int _executingOnHeartbeat; - public Heartbeat(IHeartbeatHandler[] callbacks, ISystemClock systemClock, IDebugger debugger, IKestrelTrace trace): this(callbacks, systemClock, debugger, trace, Interval) - { - - } - - internal Heartbeat(IHeartbeatHandler[] callbacks, ISystemClock systemClock, IDebugger debugger, IKestrelTrace trace, TimeSpan interval) + public Heartbeat(IHeartbeatHandler[] callbacks, ISystemClock systemClock, IDebugger debugger, IKestrelTrace trace) { _callbacks = callbacks; _systemClock = systemClock; _debugger = debugger; _trace = trace; - _interval = interval; + _interval = Interval; } public void Start() diff --git a/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs b/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs index bc54e7338587..98d358592170 100644 --- a/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs +++ b/src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs @@ -11,9 +11,10 @@ public class HeartbeatManager : IHeartbeatHandler, ISystemClock private readonly Action _walkCallback; private DateTimeOffset _now; - public HeartbeatManager(ConnectionManager connectionManager) + public HeartbeatManager(ConnectionManager connectionManager, DateTimeOffset initialUtcNow) { _connectionManager = connectionManager; + _now = initialUtcNow; _walkCallback = WalkCallback; } diff --git a/src/Kestrel.Core/KestrelServer.cs b/src/Kestrel.Core/KestrelServer.cs index cc21f738d04a..e90188a6ebd6 100644 --- a/src/Kestrel.Core/KestrelServer.cs +++ b/src/Kestrel.Core/KestrelServer.cs @@ -71,8 +71,9 @@ private static ServiceContext CreateServiceContext(IOptions(); @@ -83,7 +79,7 @@ public void GetDateHeaderValue_ReturnsUpdatedValueAfterHeartbeat() heartbeat.OnHeartbeat(); Assert.Equal(future.ToString(Rfc1123DateFormat), dateHeaderValueManager.GetDateHeaderValues().String); - Assert.True(systemClock.UtcNowCalled >= 2); + Assert.Equal(2, systemClock.UtcNowCalled); } } @@ -97,7 +93,7 @@ public void GetDateHeaderValue_ReturnsLastDateValueAfterHeartbeatDisposed() UtcNow = now }; - var dateHeaderValueManager = new DateHeaderValueManager(systemClock); + var dateHeaderValueManager = new DateHeaderValueManager(now); var testKestrelTrace = new TestKestrelTrace(); using (var heatbeat = new Heartbeat(new IHeartbeatHandler[] { dateHeaderValueManager }, systemClock, DebuggerWrapper.Singleton, testKestrelTrace)) diff --git a/test/Kestrel.InMemory.FunctionalTests/HttpsTests.cs b/test/Kestrel.InMemory.FunctionalTests/HttpsTests.cs index 9e52de9f12b9..1f3039abbbb8 100644 --- a/test/Kestrel.InMemory.FunctionalTests/HttpsTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/HttpsTests.cs @@ -312,7 +312,7 @@ public async Task HandshakeTimesOutAndIsLoggedAsDebug() LoggerFactory.AddProvider(loggerProvider); var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); var handshakeStartedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); TimeSpan handshakeTimeout = default; diff --git a/test/Kestrel.InMemory.FunctionalTests/KeepAliveTimeoutTests.cs b/test/Kestrel.InMemory.FunctionalTests/KeepAliveTimeoutTests.cs index 716ff9583a2b..f462b0716cdc 100644 --- a/test/Kestrel.InMemory.FunctionalTests/KeepAliveTimeoutTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/KeepAliveTimeoutTests.cs @@ -27,7 +27,7 @@ public class KeepAliveTimeoutTests : LoggedTest public async Task ConnectionClosedWhenKeepAliveTimeoutExpires() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -51,7 +51,7 @@ await connection.Send( public async Task ConnectionKeptAliveBetweenRequests() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -76,7 +76,7 @@ await connection.Send( public async Task ConnectionNotTimedOutWhileRequestBeingSent() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -113,7 +113,7 @@ await connection.Send( private async Task ConnectionNotTimedOutWhileAppIsRunning() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); var cts = new CancellationTokenSource(); using (var server = CreateServer(testContext, longRunningCt: cts.Token)) @@ -150,7 +150,7 @@ await connection.Send( private async Task ConnectionTimesOutWhenOpenedButNoRequestSent() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -167,7 +167,7 @@ private async Task ConnectionTimesOutWhenOpenedButNoRequestSent() private async Task KeepAliveTimeoutDoesNotApplyToUpgradedConnections() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); var cts = new CancellationTokenSource(); using (var server = CreateServer(testContext, upgradeCt: cts.Token)) diff --git a/test/Kestrel.InMemory.FunctionalTests/RequestBodyTimeoutTests.cs b/test/Kestrel.InMemory.FunctionalTests/RequestBodyTimeoutTests.cs index f8946d707e95..b0fe3d90177c 100644 --- a/test/Kestrel.InMemory.FunctionalTests/RequestBodyTimeoutTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/RequestBodyTimeoutTests.cs @@ -22,7 +22,7 @@ public async Task RequestTimesOutWhenRequestBodyNotReceivedAtSpecifiedMinimumRat { var gracePeriod = TimeSpan.FromSeconds(5); var serviceContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(serviceContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(serviceContext.ConnectionManager, serviceContext.SystemClock.UtcNow); var appRunningEvent = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); @@ -136,7 +136,7 @@ public async Task ConnectionClosedEvenIfAppSwallowsException() { var gracePeriod = TimeSpan.FromSeconds(5); var serviceContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(serviceContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(serviceContext.ConnectionManager, serviceContext.SystemClock.UtcNow); var appRunningTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var exceptionSwallowedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); diff --git a/test/Kestrel.InMemory.FunctionalTests/RequestHeadersTimeoutTests.cs b/test/Kestrel.InMemory.FunctionalTests/RequestHeadersTimeoutTests.cs index e5fd7141799e..5bd803a6dc63 100644 --- a/test/Kestrel.InMemory.FunctionalTests/RequestHeadersTimeoutTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/RequestHeadersTimeoutTests.cs @@ -26,7 +26,7 @@ public class RequestHeadersTimeoutTests : LoggedTest public async Task ConnectionAbortedWhenRequestHeadersNotReceivedInTime(string headers) { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -47,7 +47,7 @@ await connection.Send( public async Task RequestHeadersTimeoutCanceledAfterHeadersReceived() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -76,7 +76,7 @@ await connection.Send( public async Task ConnectionAbortedWhenRequestLineNotReceivedInTime(string requestLine) { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) @@ -95,7 +95,7 @@ public async Task ConnectionAbortedWhenRequestLineNotReceivedInTime(string reque public async Task TimeoutNotResetOnEachRequestLineCharacterReceived() { var testContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(testContext.ConnectionManager, testContext.SystemClock.UtcNow); using (var server = CreateServer(testContext)) using (var connection = server.CreateConnection()) diff --git a/test/Kestrel.InMemory.FunctionalTests/RequestTests.cs b/test/Kestrel.InMemory.FunctionalTests/RequestTests.cs index 34f6b92333ef..0edb95b27386 100644 --- a/test/Kestrel.InMemory.FunctionalTests/RequestTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/RequestTests.cs @@ -877,7 +877,7 @@ public async Task DoesNotEnforceRequestBodyMinimumDataRateOnUpgradedRequest() var appEvent = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var delayEvent = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var serviceContext = new TestServiceContext(LoggerFactory); - var heartbeatManager = new HeartbeatManager(serviceContext.ConnectionManager); + var heartbeatManager = new HeartbeatManager(serviceContext.ConnectionManager, serviceContext.SystemClock.UtcNow); using (var server = new TestServer(async context => { diff --git a/test/Kestrel.Transport.FunctionalTests/RequestTests.cs b/test/Kestrel.Transport.FunctionalTests/RequestTests.cs index fc5a5d382be6..e194331df104 100644 --- a/test/Kestrel.Transport.FunctionalTests/RequestTests.cs +++ b/test/Kestrel.Transport.FunctionalTests/RequestTests.cs @@ -100,7 +100,9 @@ public void LargeUpload(long contentLength, bool checkBytes) using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)) { socket.Connect(new IPEndPoint(IPAddress.Loopback, host.GetPort())); - socket.Send(Encoding.ASCII.GetBytes($"POST / HTTP/1.0\r\nContent-Length: {contentLength}\r\n\r\n")); + socket.Send(Encoding.ASCII.GetBytes("POST / HTTP/1.0\r\n")); + Thread.Sleep(5000); + socket.Send(Encoding.ASCII.GetBytes($"Content-Length: {contentLength}\r\n\r\n")); var contentBytes = new byte[bufferLength]; diff --git a/test/shared/TestServiceContext.cs b/test/shared/TestServiceContext.cs index 356164e72579..3e833a9e5ec3 100644 --- a/test/shared/TestServiceContext.cs +++ b/test/shared/TestServiceContext.cs @@ -41,16 +41,17 @@ private static KestrelTrace CreateLoggingTrace(ILoggerFactory loggerFactory) public void InitializeHeartbeat() { - MockSystemClock = null; - SystemClock = new SystemClock(); - DateHeaderValueManager = new DateHeaderValueManager(SystemClock); - - var heartbeatManager = new HeartbeatManager(ConnectionManager); + var now = DateTimeOffset.UtcNow; + var heartbeatManager = new HeartbeatManager(ConnectionManager, now); + DateHeaderValueManager = new DateHeaderValueManager(now); Heartbeat = new Heartbeat( new IHeartbeatHandler[] { DateHeaderValueManager, heartbeatManager }, - SystemClock, + new SystemClock(), DebuggerWrapper.Singleton, Log); + + MockSystemClock = null; + SystemClock = heartbeatManager; } private void Initialize(ILoggerFactory loggerFactory, IKestrelTrace kestrelTrace) @@ -60,7 +61,7 @@ private void Initialize(ILoggerFactory loggerFactory, IKestrelTrace kestrelTrace Scheduler = PipeScheduler.ThreadPool; MockSystemClock = new MockSystemClock(); SystemClock = MockSystemClock; - DateHeaderValueManager = new DateHeaderValueManager(MockSystemClock); + DateHeaderValueManager = new DateHeaderValueManager(MockSystemClock.UtcNow); ConnectionManager = new ConnectionManager(Log, ResourceCounter.Unlimited); HttpParser = new HttpParser(Log.IsEnabled(LogLevel.Information)); ServerOptions = new KestrelServerOptions