Skip to content

Commit

Permalink
Fix race when SystemClock is accessed before first heartbeat (#2851)
Browse files Browse the repository at this point in the history
  • Loading branch information
@halter73
halter73 authored Aug 27, 2018
1 parent f6cc980 commit f70ba53
Show file tree
Hide file tree
Showing 12 changed files with 41 additions and 45 deletions.
6 changes: 3 additions & 3 deletions src/Kestrel.Core/Internal/Http/DateHeaderValueManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ public class DateHeaderValueManager : IHeartbeatHandler
/// Initializes a new instance of the <see cref="DateHeaderValueManager"/> class.
/// </summary>
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);
}

/// <summary>
Expand Down
9 changes: 2 additions & 7 deletions src/Kestrel.Core/Internal/Infrastructure/Heartbeat.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
3 changes: 2 additions & 1 deletion src/Kestrel.Core/Internal/Infrastructure/HeartbeatManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ public class HeartbeatManager : IHeartbeatHandler, ISystemClock
private readonly Action<KestrelConnection> _walkCallback;
private DateTimeOffset _now;

public HeartbeatManager(ConnectionManager connectionManager)
public HeartbeatManager(ConnectionManager connectionManager, DateTimeOffset initialUtcNow)
{
_connectionManager = connectionManager;
_now = initialUtcNow;
_walkCallback = WalkCallback;
}

Expand Down
5 changes: 3 additions & 2 deletions src/Kestrel.Core/KestrelServer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ private static ServiceContext CreateServiceContext(IOptions<KestrelServerOptions
trace,
serverOptions.Limits.MaxConcurrentUpgradedConnections);

var heartbeatManager = new HeartbeatManager(connectionManager);
var dateHeaderValueManager = new DateHeaderValueManager(heartbeatManager);
var now = DateTimeOffset.UtcNow;
var heartbeatManager = new HeartbeatManager(connectionManager, now);
var dateHeaderValueManager = new DateHeaderValueManager(now);
var heartbeat = new Heartbeat(
new IHeartbeatHandler[] { dateHeaderValueManager, heartbeatManager },
new SystemClock(),
Expand Down
16 changes: 6 additions & 10 deletions test/Kestrel.Core.Tests/DateHeaderValueManagerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,8 @@ public class DateHeaderValueManagerTests
public void GetDateHeaderValue_ReturnsDateValueInRFC1123Format()
{
var now = DateTimeOffset.UtcNow;
var systemClock = new MockSystemClock
{
UtcNow = now
};

var dateHeaderValueManager = new DateHeaderValueManager(systemClock);
var dateHeaderValueManager = new DateHeaderValueManager(now);
Assert.Equal(now.ToString(Rfc1123DateFormat), dateHeaderValueManager.GetDateHeaderValues().String);
}

Expand All @@ -43,7 +39,7 @@ public void GetDateHeaderValue_ReturnsCachedValueBetweenTimerTicks()
UtcNow = now
};

var dateHeaderValueManager = new DateHeaderValueManager(systemClock);
var dateHeaderValueManager = new DateHeaderValueManager(now);
var testKestrelTrace = new TestKestrelTrace();

using (var heartbeat = new Heartbeat(new IHeartbeatHandler[] { dateHeaderValueManager }, systemClock, DebuggerWrapper.Singleton, testKestrelTrace))
Expand All @@ -53,7 +49,7 @@ public void GetDateHeaderValue_ReturnsCachedValueBetweenTimerTicks()
Assert.Equal(now.ToString(Rfc1123DateFormat), dateHeaderValueManager.GetDateHeaderValues().String);
}

Assert.Equal(1, systemClock.UtcNowCalled);
Assert.Equal(0, systemClock.UtcNowCalled);
}

[Fact]
Expand All @@ -66,7 +62,7 @@ public void GetDateHeaderValue_ReturnsUpdatedValueAfterHeartbeat()
UtcNow = now
};

var dateHeaderValueManager = new DateHeaderValueManager(systemClock);
var dateHeaderValueManager = new DateHeaderValueManager(now);
var testKestrelTrace = new TestKestrelTrace();

var mockHeartbeatHandler = new Mock<IHeartbeatHandler>();
Expand All @@ -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);
}
}

Expand All @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion test/Kestrel.InMemory.FunctionalTests/HttpsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<object>(TaskCreationOptions.RunContinuationsAsynchronously);
TimeSpan handshakeTimeout = default;
Expand Down
12 changes: 6 additions & 6 deletions test/Kestrel.InMemory.FunctionalTests/KeepAliveTimeoutTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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())
Expand All @@ -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))
Expand Down
4 changes: 2 additions & 2 deletions test/Kestrel.InMemory.FunctionalTests/RequestBodyTimeoutTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<object>(TaskCreationOptions.RunContinuationsAsynchronously);

Expand Down Expand Up @@ -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<object>(TaskCreationOptions.RunContinuationsAsynchronously);
var exceptionSwallowedTcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
Expand Down
8 changes: 4 additions & 4 deletions test/Kestrel.InMemory.FunctionalTests/RequestHeadersTimeoutTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand Down
2 changes: 1 addition & 1 deletion test/Kestrel.InMemory.FunctionalTests/RequestTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -877,7 +877,7 @@ public async Task DoesNotEnforceRequestBodyMinimumDataRateOnUpgradedRequest()
var appEvent = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
var delayEvent = new TaskCompletionSource<object>(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 =>
{
Expand Down
4 changes: 3 additions & 1 deletion test/Kestrel.Transport.FunctionalTests/RequestTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand Down
15 changes: 8 additions & 7 deletions test/shared/TestServiceContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<Http1ParsingHandler>(Log.IsEnabled(LogLevel.Information));
ServerOptions = new KestrelServerOptions
Expand Down

0 comments on commit f70ba53

Please sign in to comment.