From 631f04d656ac5f97ce339e80c8eebfe4de555449 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 6 Jun 2020 22:33:20 +1200 Subject: [PATCH] Fixes and tests --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 3 + src/Servers/Kestrel/Core/src/Http2Limits.cs | 28 ++- .../src/Internal/Http2/Http2Connection.cs | 7 +- .../Http2/Http2ConnectionKeepAlive.cs | 112 ------------ .../Core/src/Internal/Http2/Http2KeepAlive.cs | 116 ++++++++++++ .../Http2/Http2KeepAliveTests.cs | 170 +++++++++++++++++- .../Http2/Http2TestBase.cs | 13 +- 7 files changed, 320 insertions(+), 129 deletions(-) delete mode 100644 src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionKeepAlive.cs create mode 100644 src/Servers/Kestrel/Core/src/Internal/Http2/Http2KeepAlive.cs diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index a4e4c05e1f5b..50d8a7fba458 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -608,4 +608,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l Timeout while waiting for a keep alive ping acknowledgement. + + A TimeSpan value greater than {value} is required. + \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Http2Limits.cs b/src/Servers/Kestrel/Core/src/Http2Limits.cs index c7c2d2f8472c..791d2d714b30 100644 --- a/src/Servers/Kestrel/Core/src/Http2Limits.cs +++ b/src/Servers/Kestrel/Core/src/Http2Limits.cs @@ -12,6 +12,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core /// public class Http2Limits { + private static readonly TimeSpan MinimumKeepAliveInterval = TimeSpan.FromSeconds(1); + private int _maxStreamsPerConnection = 100; private int _headerTableSize = (int)Http2PeerSettings.DefaultHeaderTableSize; private int _maxFrameSize = (int)Http2PeerSettings.DefaultMaxFrameSize; @@ -145,28 +147,46 @@ public int InitialStreamWindowSize } } + /// + /// Gets or sets the keep alive ping interval. The server will send a keep alive ping to the client if it + /// doesn't see any activity after this interval elapses. This property is used together with + /// to close inactive connections. + /// + /// Value must be greater than 1 second. Set to null or to disable + /// the keep alive ping interval. Defaults to null. + /// + /// public TimeSpan? KeepAlivePingInterval { get => _keepAlivePingInterval; set { - if (value <= TimeSpan.Zero && value != Timeout.InfiniteTimeSpan) + if (value < MinimumKeepAliveInterval && value != Timeout.InfiniteTimeSpan) { - throw new ArgumentOutOfRangeException(nameof(value), CoreStrings.PositiveTimeSpanRequired); + throw new ArgumentOutOfRangeException(nameof(value), CoreStrings.FormatArgumentTimeSpanGreaterThan(MinimumKeepAliveInterval)); } _keepAlivePingInterval = value; } } + /// + /// Gets or sets the keep alive ping timeout. Keep alive pings are sent when a period of inactivity exceeds + /// the configured value. The server will close the connection if it + /// doesn't receive an acknowledgement of the keep alive ping within the timeout. + /// + /// Value must be greater than 1 second. Set to to disable the keep + /// alive ping timeout. Defaults to 20 seconds. + /// + /// public TimeSpan KeepAlivePingTimeout { get => _keepAlivePingTimeout; set { - if (value <= TimeSpan.Zero && value != Timeout.InfiniteTimeSpan) + if (value < MinimumKeepAliveInterval && value != Timeout.InfiniteTimeSpan) { - throw new ArgumentOutOfRangeException(nameof(value), CoreStrings.PositiveTimeSpanRequired); + throw new ArgumentOutOfRangeException(nameof(value), CoreStrings.FormatArgumentTimeSpanGreaterThan(MinimumKeepAliveInterval)); } _keepAlivePingTimeout = value; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index c94c87a9f10d..b814e00261be 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -41,7 +41,6 @@ internal partial class Http2Connection : IHttp2StreamLifetimeHandler, IHttpHeade private readonly HPackDecoder _hpackDecoder; private readonly InputFlowControl _inputFlowControl; private readonly OutputFlowControl _outputFlowControl = new OutputFlowControl(new MultipleAwaitableProvider(), Http2PeerSettings.DefaultInitialWindowSize); - private readonly Http2ConnectionKeepAlive _keepAlive; private readonly Http2PeerSettings _serverSettings = new Http2PeerSettings(); private readonly Http2PeerSettings _clientSettings = new Http2PeerSettings(); @@ -67,6 +66,7 @@ internal partial class Http2Connection : IHttp2StreamLifetimeHandler, IHttpHeade private int _isClosed; // Internal for testing + internal readonly Http2KeepAlive _keepAlive; internal readonly Dictionary _streams = new Dictionary(); internal Http2StreamStack StreamPool; @@ -109,7 +109,7 @@ public Http2Connection(HttpConnectionContext context) if (http2Limits.KeepAlivePingInterval != null && http2Limits.KeepAlivePingInterval != Timeout.InfiniteTimeSpan) { - _keepAlive = new Http2ConnectionKeepAlive( + _keepAlive = new Http2KeepAlive( http2Limits.KeepAlivePingInterval.GetValueOrDefault(), http2Limits.KeepAlivePingTimeout, context.ServiceContext.SystemClock); @@ -223,7 +223,8 @@ public async Task ProcessRequestsAsync(IHttpApplication appl var state = _keepAlive.ProcessKeepAlive(!result.IsCanceled); if (state == KeepAliveState.SendPing) { - await _frameWriter.WritePingAsync(Http2PingFrameFlags.NONE, Http2ConnectionKeepAlive.PingPayload); + await _frameWriter.WritePingAsync(Http2PingFrameFlags.NONE, Http2KeepAlive.PingPayload); + _keepAlive.PingSent(); } else if (state == KeepAliveState.Timeout) { diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionKeepAlive.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionKeepAlive.cs deleted file mode 100644 index e4960338dcbe..000000000000 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2ConnectionKeepAlive.cs +++ /dev/null @@ -1,112 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.Buffers; -using System.Diagnostics; -using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; - -namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 -{ - internal enum KeepAliveState - { - None, - SendPing, - PingSent, - Timeout - } - - internal class Http2ConnectionKeepAlive - { - internal static readonly ReadOnlySequence PingPayload = new ReadOnlySequence(new byte[8]); - - private readonly TimeSpan _keepAliveInterval; - private readonly TimeSpan _keepAliveTimeout; - private readonly ISystemClock _systemClock; - private KeepAliveState _state; - private bool _bytesReceivedCurrentTick; - private long _lastBytesReceivedTimestamp; - private long _pingSentTimestamp; - - public Http2ConnectionKeepAlive(TimeSpan keepAliveInterval, TimeSpan keepAliveTimeout, ISystemClock systemClock) - { - _keepAliveInterval = keepAliveInterval; - _keepAliveTimeout = keepAliveTimeout; - _systemClock = systemClock; - } - - internal KeepAliveState ProcessKeepAlive(bool dataReceived) - { - switch (_state) - { - case KeepAliveState.None: - // No ping has been sent. - // If data has been received then update the timestamp - if (dataReceived) - { - _bytesReceivedCurrentTick = true; - } - return KeepAliveState.None; - case KeepAliveState.SendPing: - // keepAliveInterval has been exceeded and a ping has been scheduled. - // Caller of this method will send a ping so update state to PingSent. - _state = KeepAliveState.PingSent; - _pingSentTimestamp = _systemClock.UtcNowTicks; - return KeepAliveState.SendPing; - case KeepAliveState.PingSent: - return KeepAliveState.PingSent; - case KeepAliveState.Timeout: - return KeepAliveState.Timeout; - } - - Debug.Fail("Should never reach here."); - return _state; - } - - public void PingAckReceived() - { - if (_state == KeepAliveState.PingSent) - { - _pingSentTimestamp = 0; - _state = KeepAliveState.None; - } - } - - public void Tick(DateTimeOffset now) - { - var timestamp = now.Ticks; - - // Bytes were received since the last tick. - // Update a timestamp of when bytes were last received. - if (_bytesReceivedCurrentTick) - { - _lastBytesReceivedTimestamp = timestamp; - _bytesReceivedCurrentTick = false; - } - - switch (_state) - { - case KeepAliveState.None: - // Check whether keep alive interval has passed since last bytes received - if (timestamp > (_lastBytesReceivedTimestamp + _keepAliveInterval.Ticks)) - { - _state = KeepAliveState.SendPing; - } - return; - case KeepAliveState.SendPing: - return; - case KeepAliveState.PingSent: - if (timestamp > (_pingSentTimestamp + _keepAliveTimeout.Ticks)) - { - _pingSentTimestamp = 0; - _state = KeepAliveState.Timeout; - } - return; - case KeepAliveState.Timeout: - return; - } - - Debug.Fail("Should never reach here."); - } - } -} diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2KeepAlive.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2KeepAlive.cs new file mode 100644 index 000000000000..67f95ad37915 --- /dev/null +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2KeepAlive.cs @@ -0,0 +1,116 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Buffers; +using System.Diagnostics; +using System.Threading; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 +{ + internal enum KeepAliveState + { + None, + SendPing, + PingSent, + Timeout + } + + internal class Http2KeepAlive + { + internal static readonly ReadOnlySequence PingPayload = new ReadOnlySequence(new byte[8]); + + private readonly TimeSpan _keepAliveInterval; + private readonly TimeSpan _keepAliveTimeout; + private readonly ISystemClock _systemClock; + private readonly object _lock = new object(); + private KeepAliveState _state; + private bool _bytesReceivedCurrentTick; + private long _lastBytesReceivedTimestamp; + private long _pingSentTimestamp; + + public Http2KeepAlive(TimeSpan keepAliveInterval, TimeSpan keepAliveTimeout, ISystemClock systemClock) + { + _keepAliveInterval = keepAliveInterval; + _keepAliveTimeout = keepAliveTimeout; + _systemClock = systemClock; + } + + internal KeepAliveState ProcessKeepAlive(bool dataReceived) + { + lock (_lock) + { + if (dataReceived) + { + _bytesReceivedCurrentTick = true; + } + return _state; + } + } + + public void PingSent() + { + lock (_lock) + { + _state = KeepAliveState.PingSent; + _pingSentTimestamp = _systemClock.UtcNowTicks; + } + } + + public void PingAckReceived() + { + lock (_lock) + { + if (_state == KeepAliveState.PingSent) + { + _pingSentTimestamp = 0; + _state = KeepAliveState.None; + } + } + } + + public void Tick(DateTimeOffset now) + { + var timestamp = now.Ticks; + + lock (_lock) + { + // Bytes were received since the last tick. + // Update a timestamp of when bytes were last received. + if (_bytesReceivedCurrentTick) + { + _lastBytesReceivedTimestamp = timestamp; + _bytesReceivedCurrentTick = false; + } + + switch (_state) + { + case KeepAliveState.None: + // Check whether keep alive interval has passed since last bytes received + if (timestamp > (_lastBytesReceivedTimestamp + _keepAliveInterval.Ticks)) + { + _state = KeepAliveState.SendPing; + } + return; + case KeepAliveState.SendPing: + return; + case KeepAliveState.PingSent: + if (_keepAliveTimeout != Timeout.InfiniteTimeSpan) + { + if (timestamp > (_pingSentTimestamp + _keepAliveTimeout.Ticks)) + { + _pingSentTimestamp = 0; + _state = KeepAliveState.Timeout; + } + } + return; + case KeepAliveState.Timeout: + return; + } + } + + Debug.Fail("Should never reach here."); + } + } +} diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2KeepAliveTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2KeepAliveTests.cs index bd4a68708b51..8ce21ae442aa 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2KeepAliveTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2KeepAliveTests.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; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2; using Xunit; @@ -11,7 +12,48 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests public class Http2KeepAliveTests : Http2TestBase { [Fact] - public async Task IntervalExceededWithoutActivity_PingSent() + public async Task KeepAlivePingInterval_Infinite_KeepAliveNotEnabled() + { + _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = Timeout.InfiniteTimeSpan; + + await InitializeConnectionAsync(_noopApplication); + + Assert.Null(_connection._keepAlive); + + await StopConnectionAsync(expectedLastStreamId: 0, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task KeepAlivePingTimeout_Infinite_NoGoAway() + { + _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); + _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingTimeout = Timeout.InfiniteTimeSpan; + + await InitializeConnectionAsync(_noopApplication); + + DateTimeOffset now = new DateTimeOffset(1, TimeSpan.Zero); + + // Heartbeat + TriggerTick(now); + + // Heartbeat that exceeds interval + TriggerTick(now + TimeSpan.FromSeconds(1.1)); + + await ExpectAsync(Http2FrameType.PING, + withLength: 8, + withFlags: (byte)Http2PingFrameFlags.NONE, + withStreamId: 0); + + // Heartbeat that exceeds timeout + TriggerTick(now + TimeSpan.FromSeconds(1.1 * 2)); + TriggerTick(now + TimeSpan.FromSeconds(1.1 * 3)); + TriggerTick(now + TimeSpan.FromSeconds(1.1 * 20)); + + await StopConnectionAsync(expectedLastStreamId: 0, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task IntervalExceeded_WithoutActivity_PingSent() { _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); @@ -34,7 +76,7 @@ await ExpectAsync(Http2FrameType.PING, } [Fact] - public async Task IntervalExceededWithActivity_NoPingSent() + public async Task IntervalExceeded_WithActivity_NoPingSent() { _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); @@ -76,7 +118,7 @@ public async Task IntervalNotExceeded_NoPingSent() } [Fact] - public async Task IntervalExceededMultipleTimes_PingsNotSentWhileAwaitingOnAck() + public async Task IntervalExceeded_MultipleTimes_PingsNotSentWhileAwaitingOnAck() { _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); @@ -100,7 +142,7 @@ await ExpectAsync(Http2FrameType.PING, } [Fact] - public async Task IntervalExceededMultipleTimes_PingSentAfterAck() + public async Task IntervalExceeded_MultipleTimes_PingSentAfterAck() { _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); @@ -139,7 +181,7 @@ await ExpectAsync(Http2FrameType.PING, } [Fact] - public async Task TimeoutExceededWithoutAck_GoAway() + public async Task TimeoutExceeded_NoAck_GoAway() { _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingTimeout = TimeSpan.FromSeconds(3); @@ -166,5 +208,123 @@ await ExpectAsync(Http2FrameType.PING, VerifyGoAway(await ReceiveFrameAsync(), 0, Http2ErrorCode.CANCEL); } + + [Fact] + public async Task IntervalExceeded_StreamStarted_NoPingSent() + { + _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); + + await InitializeConnectionAsync(_noopApplication); + + DateTimeOffset now = new DateTimeOffset(1, TimeSpan.Zero); + + // Heartbeat + TriggerTick(now); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: true); + + // Heartbeat that exceeds interval + TriggerTick(now + TimeSpan.FromSeconds(1.1)); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task IntervalExceeded_ConnectionFlowControlUsedUpThenPings_NoPingSent() + { + _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); + + // Reduce connection window size so that one stream can fill it + _serviceContext.ServerOptions.Limits.Http2.InitialConnectionWindowSize = 65535; + + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await InitializeConnectionAsync(async c => + { + // Don't consume any request data + await tcs.Task; + }, expectedWindowUpdate: false); + + DateTimeOffset now = new DateTimeOffset(1, TimeSpan.Zero); + + // Heartbeat + TriggerTick(now); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + + // Use up connection flow control + await SendDataAsync(1, new byte[16384], false); + await SendDataAsync(1, new byte[16384], false); + await SendDataAsync(1, new byte[16384], false); + await SendDataAsync(1, new byte[16383], false); + + // Heartbeat that exceeds interval + TriggerTick(now + TimeSpan.FromSeconds(1.1)); + + // Send ping that will update the keep alive on the server + await SendPingAsync(Http2PingFrameFlags.NONE); + await ExpectAsync(Http2FrameType.PING, + withLength: 8, + withFlags: (byte)Http2PingFrameFlags.ACK, + withStreamId: 0); + + // Heartbeat that exceeds interval + TriggerTick(now + TimeSpan.FromSeconds(1.1 * 2)); + + // Continue request delegate on server + tcs.SetResult(null); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + } + + [Fact] + public async Task TimeoutExceeded_ConnectionFlowControlUsedUpThenPings_NoGoAway() + { + _serviceContext.ServerOptions.Limits.Http2.KeepAlivePingInterval = TimeSpan.FromSeconds(1); + + // Reduce connection window size so that one stream can fill it + _serviceContext.ServerOptions.Limits.Http2.InitialConnectionWindowSize = 65535; + + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await InitializeConnectionAsync(async c => + { + // Don't consume any request data + await tcs.Task; + }, expectedWindowUpdate: false); + + DateTimeOffset now = new DateTimeOffset(1, TimeSpan.Zero); + + // Heartbeat + TriggerTick(now); + + await StartStreamAsync(1, _browserRequestHeaders, endStream: false); + + // Use up connection flow control + await SendDataAsync(1, new byte[16384], false); + await SendDataAsync(1, new byte[16384], false); + await SendDataAsync(1, new byte[16384], false); + await SendDataAsync(1, new byte[16383], false); + + // Heartbeat that exceeds interval + TriggerTick(now + TimeSpan.FromSeconds(1.1)); + + // Heartbeat that triggers keep alive ping + TriggerTick(now + TimeSpan.FromSeconds(1.1 * 2)); + + await ExpectAsync(Http2FrameType.PING, + withLength: 8, + withFlags: (byte)Http2PingFrameFlags.NONE, + withStreamId: 0); + + // Send ping ack that will reset the keep alive on the server + await SendPingAsync(Http2PingFrameFlags.ACK); + + // Heartbeat that exceeds interval + TriggerTick(now + TimeSpan.FromSeconds(1.1 * 3)); + + // Continue request delegate on server + tcs.SetResult(null); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs index 5365a5d2f0ea..1a9cbba99883 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs @@ -441,7 +441,7 @@ protected void CreateConnection() .Callback(r => httpConnection.OnTimeout(r)); } - protected async Task InitializeConnectionAsync(RequestDelegate application, int expectedSettingsCount = 3) + protected async Task InitializeConnectionAsync(RequestDelegate application, int expectedSettingsCount = 3, bool expectedWindowUpdate = true) { if (_connection == null) { @@ -473,10 +473,13 @@ await ExpectAsync(Http2FrameType.SETTINGS, withFlags: 0, withStreamId: 0); - await ExpectAsync(Http2FrameType.WINDOW_UPDATE, - withLength: 4, - withFlags: 0, - withStreamId: 0); + if (expectedWindowUpdate) + { + await ExpectAsync(Http2FrameType.WINDOW_UPDATE, + withLength: 4, + withFlags: 0, + withStreamId: 0); + } await ExpectAsync(Http2FrameType.SETTINGS, withLength: 0,