From 065f0d001cd0c5c70860b8d12d95ada63b1bf4ea Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 4 Mar 2020 08:37:31 +1300 Subject: [PATCH] Only reset Http2Stream on stream completion (#19431) --- .../Core/src/Internal/Http/Http1Connection.cs | 2 +- .../Core/src/Internal/Http/HttpProtocol.cs | 7 ++- .../Core/src/Internal/Http2/Http2Stream.cs | 18 +++++-- .../Core/src/Internal/Http2/Http2StreamOfT.cs | 2 +- .../Core/src/Internal/Http3/Http3Stream.cs | 2 +- .../HttpProtocolFeatureCollectionTests.cs | 2 +- .../Http2/Http2ConnectionTests.cs | 47 +++++++++++++++++++ 7 files changed, 71 insertions(+), 9 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs index 832d48cdce..ab341a8e53 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs @@ -45,7 +45,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http public Http1Connection(HttpConnectionContext context) { - Initialize(context); + Initialize(context, reset: true); _context = context; _parser = ServiceContext.HttpParser; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs index 539c386efa..87dc89f6d3 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs @@ -76,13 +76,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http private Stream _requestStreamInternal; private Stream _responseStreamInternal; - public void Initialize(HttpConnectionContext context) + public void Initialize(HttpConnectionContext context, bool reset) { _context = context; ServerOptions = ServiceContext.ServerOptions; - Reset(); + if (reset) + { + Reset(); + } HttpResponseControl = this; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index df0190cba5..9853a184a0 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -33,9 +33,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 private StreamCompletionFlags _completionState; private readonly object _completionLock = new object(); - public void Initialize(Http2StreamContext context) + /// + /// Initialize the stream with the specified context. + /// + /// The context. + /// + /// A value indicating whether to reset the protocol instance. + /// We want to reset when the stream is created, and when the stream is returned to the pool. + /// The stream shouldn't be reset when fetched from the pool. + /// + public void InitializePooled(Http2StreamContext context, bool reset) { - base.Initialize(context); + base.Initialize(context, reset); _decrementCalled = false; _completionState = StreamCompletionFlags.None; @@ -71,7 +80,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public void InitializeWithExistingContext(int streamId) { _context.StreamId = streamId; - Initialize(_context); + + // The stream was reset when it was completed. + // We don't want to reset it twice because reused headers will be discarded. + InitializePooled(_context, reset: false); } public int StreamId => _context.StreamId; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2StreamOfT.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2StreamOfT.cs index 2ba223f43c..ef6c790f1d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2StreamOfT.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2StreamOfT.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 public Http2Stream(IHttpApplication application, Http2StreamContext context) { - Initialize(context); + InitializePooled(context, reset: true); _application = application; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 6518d00afd..3a4abf8590 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -52,7 +52,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3 public Http3Stream(Http3Connection http3Connection, Http3StreamContext context) { - Initialize(context); + Initialize(context, reset: true); InputRemaining = null; diff --git a/src/Servers/Kestrel/Core/test/HttpProtocolFeatureCollectionTests.cs b/src/Servers/Kestrel/Core/test/HttpProtocolFeatureCollectionTests.cs index f62bad9117..789d93dfe5 100644 --- a/src/Servers/Kestrel/Core/test/HttpProtocolFeatureCollectionTests.cs +++ b/src/Servers/Kestrel/Core/test/HttpProtocolFeatureCollectionTests.cs @@ -251,7 +251,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { public TestHttp2Stream(Http2StreamContext context) { - Initialize(context); + InitializePooled(context, reset: true); } public override void Execute() diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs index b0007a6a0d..486e046b7f 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs @@ -25,6 +25,53 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { public class Http2ConnectionTests : Http2TestBase { + [Fact] + public async Task RequestHeaderStringReuse_MultipleStreams_KnownHeaderReused() + { + IEnumerable> requestHeaders = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/hello"), + new KeyValuePair(HeaderNames.Scheme, "http"), + new KeyValuePair(HeaderNames.Authority, "localhost:80"), + new KeyValuePair(HeaderNames.ContentType, "application/json") + }; + + await InitializeConnectionAsync(_readHeadersApplication); + + await StartStreamAsync(1, requestHeaders, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)(Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM), + withStreamId: 1); + + var contentType1 = _receivedHeaders["Content-Type"]; + + // Ping will trigger the stream to be returned to the pool so we can assert it + await SendPingAsync(Http2PingFrameFlags.NONE); + await ExpectAsync(Http2FrameType.PING, + withLength: 8, + withFlags: (byte)Http2PingFrameFlags.ACK, + withStreamId: 0); + + // Stream has been returned to the pool + Assert.Equal(1, _connection.StreamPool.Count); + + await StartStreamAsync(3, requestHeaders, endStream: true); + + await ExpectAsync(Http2FrameType.HEADERS, + withLength: 55, + withFlags: (byte)(Http2HeadersFrameFlags.END_HEADERS | Http2HeadersFrameFlags.END_STREAM), + withStreamId: 3); + + var contentType2 = _receivedHeaders["Content-Type"]; + + Assert.Same(contentType1, contentType2); + + await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); + } + [Fact] public async Task StreamPool_SingleStream_ReturnedToPool() {