Cache _absoluteRequestTarget when reusing strings (#18547)

This commit is contained in:
Stephen Halter 2020-02-14 09:38:31 -08:00 committed by GitHub
parent f3e2b4d4d1
commit 439f4af49c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 49 additions and 8 deletions

View File

@ -34,6 +34,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
private HttpRequestTarget _requestTargetForm = HttpRequestTarget.Unknown; private HttpRequestTarget _requestTargetForm = HttpRequestTarget.Unknown;
private Uri _absoluteRequestTarget; private Uri _absoluteRequestTarget;
// The _parsed fields cache the Path, QueryString, RawTarget, and/or _absoluteRequestTarget
// from the previous request when DisableStringReuse is false.
private string _parsedPath = null;
private string _parsedQueryString = null;
private string _parsedRawTarget = null;
private Uri _parsedAbsoluteRequestTarget;
private int _remainingRequestHeadersBytesAllowed; private int _remainingRequestHeadersBytesAllowed;
public Http1Connection(HttpConnectionContext context) public Http1Connection(HttpConnectionContext context)
@ -337,6 +344,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
// Clear parsedData as we won't check it if we come via this path again, // Clear parsedData as we won't check it if we come via this path again,
// an setting to null is fast as it doesn't need to use a GC write barrier. // an setting to null is fast as it doesn't need to use a GC write barrier.
_parsedRawTarget = _parsedPath = _parsedQueryString = null; _parsedRawTarget = _parsedPath = _parsedQueryString = null;
_parsedAbsoluteRequestTarget = null;
return; return;
} }
@ -389,6 +397,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
Path = _parsedPath; Path = _parsedPath;
QueryString = _parsedQueryString; QueryString = _parsedQueryString;
} }
// Clear parsedData for absolute target as we won't check it if we come via this path again,
// an setting to null is fast as it doesn't need to use a GC write barrier.
_parsedAbsoluteRequestTarget = null;
} }
catch (InvalidOperationException) catch (InvalidOperationException)
{ {
@ -441,9 +453,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
Path = string.Empty; Path = string.Empty;
QueryString = string.Empty; QueryString = string.Empty;
// Clear parsedData for path and queryString as we won't check it if we come via this path again, // Clear parsedData for path, queryString and absolute target as we won't check it if we come via this path again,
// an setting to null is fast as it doesn't need to use a GC write barrier. // an setting to null is fast as it doesn't need to use a GC write barrier.
_parsedPath = _parsedQueryString = null; _parsedPath = _parsedQueryString = null;
_parsedAbsoluteRequestTarget = null;
} }
private void OnAsteriskFormTarget(HttpMethod method) private void OnAsteriskFormTarget(HttpMethod method)
@ -463,6 +476,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
// Clear parsedData as we won't check it if we come via this path again, // Clear parsedData as we won't check it if we come via this path again,
// an setting to null is fast as it doesn't need to use a GC write barrier. // an setting to null is fast as it doesn't need to use a GC write barrier.
_parsedRawTarget = _parsedPath = _parsedQueryString = null; _parsedRawTarget = _parsedPath = _parsedQueryString = null;
_parsedAbsoluteRequestTarget = null;
} }
private void OnAbsoluteFormTarget(Span<byte> target, Span<byte> query) private void OnAbsoluteFormTarget(Span<byte> target, Span<byte> query)
@ -497,7 +511,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
ThrowRequestTargetRejected(target); ThrowRequestTargetRejected(target);
} }
_absoluteRequestTarget = uri; _absoluteRequestTarget = _parsedAbsoluteRequestTarget = uri;
Path = _parsedPath = uri.LocalPath; Path = _parsedPath = uri.LocalPath;
// don't use uri.Query because we need the unescaped version // don't use uri.Query because we need the unescaped version
previousValue = _parsedQueryString; previousValue = _parsedQueryString;
@ -520,6 +534,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
RawTarget = _parsedRawTarget; RawTarget = _parsedRawTarget;
Path = _parsedPath; Path = _parsedPath;
QueryString = _parsedQueryString; QueryString = _parsedQueryString;
_absoluteRequestTarget = _parsedAbsoluteRequestTarget;
} }
} }

View File

@ -134,13 +134,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
public HttpMethod Method { get; set; } public HttpMethod Method { get; set; }
public string PathBase { get; set; } public string PathBase { get; set; }
protected string _parsedPath = null;
public string Path { get; set; } public string Path { get; set; }
protected string _parsedQueryString = null;
public string QueryString { get; set; } public string QueryString { get; set; }
protected string _parsedRawTarget = null;
public string RawTarget { get; set; } public string RawTarget { get; set; }
public string HttpVersion public string HttpVersion

View File

@ -253,6 +253,38 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests
} }
} }
[Fact]
public async Task CanHandleTwoAbsoluteFormRequestsInARow()
{
// Regression test for https://github.com/dotnet/aspnetcore/issues/18438
var testContext = new TestServiceContext(LoggerFactory);
await using (var server = new TestServer(TestApp.EchoAppChunked, testContext))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"GET http://localhost/ HTTP/1.1",
"Host: localhost",
"",
"GET http://localhost/ HTTP/1.1",
"Host: localhost",
"",
"");
await connection.Receive(
"HTTP/1.1 200 OK",
$"Date: {testContext.DateHeaderValue}",
"Content-Length: 0",
"",
"HTTP/1.1 200 OK",
$"Date: {testContext.DateHeaderValue}",
"Content-Length: 0",
"",
"");
}
}
}
[Fact] [Fact]
public async Task AppCanSetTraceIdentifier() public async Task AppCanSetTraceIdentifier()
{ {
@ -358,7 +390,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.InMemory.FunctionalTests
} }
} }
[Fact] [Fact]
public async Task Http10NotKeptAliveByDefault() public async Task Http10NotKeptAliveByDefault()
{ {