Skip to content

Commit eec64d3

Browse files
halter73halter73
committed
Address PR feedback
1 parent 8bbb7f2 commit eec64d3

File tree

7 files changed

+33
-55
lines changed

7 files changed

+33
-55
lines changed

‎src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,6 @@ private void AppendContentLengthCustomEncoding(ReadOnlySpan<byte> value, Encodin
118118
_contentLength = parsed;
119119
}
120120

121-
122121
[MethodImpl(MethodImplOptions.NoInlining)]
123122
private void SetValueUnknown(string key, StringValues value)
124123
{

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ internal static partial class HttpUtilities
2828
private const ulong _http10VersionLong = 3471766442030158920; // GetAsciiStringAsLong("HTTP/1.0"); const results in better codegen
2929
private const ulong _http11VersionLong = 3543824036068086856; // GetAsciiStringAsLong("HTTP/1.1"); const results in better codegen
3030

31+
private static readonly UTF8EncodingSealed DefaultRequestHeaderEncoding = new UTF8EncodingSealed();
3132
private static readonly SpanAction<char, IntPtr> _getHeaderName = GetHeaderName;
3233
private static readonly SpanAction<char, IntPtr> _getAsciiStringNonNullCharacters = GetAsciiStringNonNullCharacters;
3334

@@ -121,7 +122,7 @@ public static unsafe string GetAsciiStringNonNullCharacters(this ReadOnlySpan<by
121122
}
122123

123124
public static string GetAsciiOrUTF8StringNonNullCharacters(this ReadOnlySpan<byte> span)
124-
=> StringUtilities.GetAsciiOrUTF8StringNonNullCharacters(span, KestrelServerOptions.DefaultRequestHeaderEncoding);
125+
=> StringUtilities.GetAsciiOrUTF8StringNonNullCharacters(span, DefaultRequestHeaderEncoding);
125126

126127
private static unsafe void GetAsciiStringNonNullCharacters(Span<char> buffer, IntPtr state)
127128
{
@@ -140,14 +141,14 @@ public static string GetRequestHeaderString(this ReadOnlySpan<byte> span, string
140141
{
141142
if (ReferenceEquals(KestrelServerOptions.DefaultRequestHeaderEncodingSelector, encodingSelector))
142143
{
143-
return span.GetAsciiOrUTF8StringNonNullCharacters(KestrelServerOptions.DefaultRequestHeaderEncoding);
144+
return span.GetAsciiOrUTF8StringNonNullCharacters(DefaultRequestHeaderEncoding);
144145
}
145146

146147
var encoding = encodingSelector(name);
147148

148149
if (encoding is null)
149150
{
150-
return span.GetAsciiOrUTF8StringNonNullCharacters(KestrelServerOptions.DefaultRequestHeaderEncoding);
151+
return span.GetAsciiOrUTF8StringNonNullCharacters(DefaultRequestHeaderEncoding);
151152
}
152153

153154
if (ReferenceEquals(encoding, Encoding.Latin1))
@@ -552,5 +553,13 @@ private static bool IsHex(char ch)
552553
// Check if less than 6 representing chars 'a' - 'f'
553554
|| (uint)((ch | 32) - 'a') < 6u;
554555
}
556+
557+
// Allow for de-virtualization (see https://github.com/dotnet/coreclr/pull/9230)
558+
private sealed class UTF8EncodingSealed : UTF8Encoding
559+
{
560+
public UTF8EncodingSealed() : base(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true) { }
561+
562+
public override byte[] GetPreamble() => Array.Empty<byte>();
563+
}
555564
}
556565
}

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/UTF8EncodingSealed.cs

Lines changed: 0 additions & 16 deletions
This file was deleted.

‎src/Servers/Kestrel/Core/src/KestrelServer.cs

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,11 @@
55
using System.Collections.Generic;
66
using System.IO.Pipelines;
77
using System.Linq;
8-
using System.Text;
98
using System.Threading;
109
using System.Threading.Tasks;
1110
using Microsoft.AspNetCore.Connections;
1211
using Microsoft.AspNetCore.Hosting.Server;
1312
using Microsoft.AspNetCore.Hosting.Server.Features;
14-
using Microsoft.AspNetCore.Http;
1513
using Microsoft.AspNetCore.Http.Features;
1614
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
1715
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
@@ -374,11 +372,6 @@ private void ValidateOptions()
374372
throw new InvalidOperationException(
375373
CoreStrings.FormatMaxRequestBufferSmallerThanRequestHeaderBuffer(Options.Limits.MaxRequestBufferSize.Value, Options.Limits.MaxRequestHeadersTotalSize));
376374
}
377-
378-
if (Options.RequestHeaderEncodingSelector is null)
379-
{
380-
throw new InvalidOperationException($"{nameof(KestrelServerOptions)}.{nameof(KestrelServerOptions.RequestHeaderEncodingSelector)} must not be set to null.");
381-
}
382375
}
383376

384377
private static ConnectionDelegate EnforceConnectionLimit(ConnectionDelegate innerDelegate, long? connectionLimit, IKestrelTrace trace)

‎src/Servers/Kestrel/Core/src/KestrelServerOptions.cs

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
using Microsoft.AspNetCore.Certificates.Generation;
1414
using Microsoft.AspNetCore.Http;
1515
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
16-
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
1716
using Microsoft.AspNetCore.Server.Kestrel.Https;
1817
using Microsoft.Extensions.Configuration;
1918
using Microsoft.Extensions.DependencyInjection;
@@ -26,11 +25,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core
2625
/// </summary>
2726
public class KestrelServerOptions
2827
{
29-
// Internal to fast-path header decoding when RequestHeaderEncodingSelector is unchanged.
30-
internal static readonly UTF8EncodingSealed DefaultRequestHeaderEncoding = new UTF8EncodingSealed();
31-
internal static readonly Func<string, Encoding> DefaultRequestHeaderEncodingSelector = _ => DefaultRequestHeaderEncoding;
28+
// internal to fast-path header decoding when RequestHeaderEncodingSelector is unchanged.
29+
internal static readonly Func<string, Encoding?> DefaultRequestHeaderEncodingSelector = _ => null;
3230
internal static readonly Func<string, Encoding> DefaultLatin1RequestHeaderEncodingSelector = _ => Encoding.Latin1;
3331

32+
private Func<string, Encoding?> _requestHeaderEncodingSelector = DefaultRequestHeaderEncodingSelector;
33+
3434
// The following two lists configure the endpoints that Kestrel should listen to. If both lists are empty, the "urls" config setting (e.g. UseUrls) is used.
3535
internal List<ListenOptions> CodeBackedListenOptions { get; } = new List<ListenOptions>();
3636
internal List<ListenOptions> ConfigurationBackedListenOptions { get; } = new List<ListenOptions>();
@@ -86,10 +86,11 @@ public class KestrelServerOptions
8686
/// Gets or sets a callback that returns the <see cref="Encoding"/> to decode the value for the specified request header name,
8787
/// or <see langword="null"/> to use the default <see cref="UTF8Encoding"/>.
8888
/// </summary>
89-
/// <remarks>
90-
/// Defaults to returning a <see langword="null"/> for all headers.
91-
/// </remarks>
92-
public Func<string, Encoding?> RequestHeaderEncodingSelector { get; set; } = DefaultRequestHeaderEncodingSelector;
89+
public Func<string, Encoding?> RequestHeaderEncodingSelector
90+
{
91+
get => _requestHeaderEncodingSelector;
92+
set => _requestHeaderEncodingSelector = value ?? throw new ArgumentNullException(nameof(value));
93+
}
9394

9495
/// <summary>
9596
/// Enables the Listen options callback to resolve and use services registered by the application during startup.
@@ -144,13 +145,12 @@ public void ConfigureEndpointDefaults(Action<ListenOptions> configureOptions)
144145

145146
internal Func<string, Encoding?> GetRequestHeaderEncodingSelector()
146147
{
147-
if (ReferenceEquals(RequestHeaderEncodingSelector, DefaultRequestHeaderEncodingSelector) && Latin1RequestHeaders)
148+
if (ReferenceEquals(_requestHeaderEncodingSelector, DefaultRequestHeaderEncodingSelector) && Latin1RequestHeaders)
148149
{
149150
return DefaultLatin1RequestHeaderEncodingSelector;
150151
}
151152

152-
return RequestHeaderEncodingSelector
153-
?? throw new InvalidOperationException($"{nameof(KestrelServerOptions)}.{nameof(RequestHeaderEncodingSelector)} must not be set to null.");
153+
return _requestHeaderEncodingSelector;
154154
}
155155

156156
internal void ApplyEndpointDefaults(ListenOptions listenOptions)
@@ -256,7 +256,6 @@ private void EnsureDefaultCert()
256256
/// This will only reload endpoints defined in the "Endpoints" section of your <paramref name="config"/>. Endpoints defined in code will not be reloaded.
257257
/// </param>
258258
/// <returns>A <see cref="KestrelConfigurationLoader"/> for further endpoint configuration.</returns>
259-
260259
public KestrelConfigurationLoader Configure(IConfiguration config, bool reloadOnChange)
261260
{
262261
var loader = new KestrelConfigurationLoader(this, config, reloadOnChange);

‎src/Servers/Kestrel/Core/test/KestrelServerOptionsTests.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Net;
56
using Xunit;
67

@@ -60,5 +61,14 @@ public void CanCallListenAfterConfigure()
6061
// https://github.com/dotnet/aspnetcore/issues/21423
6162
options.ListenLocalhost(5000);
6263
}
64+
65+
[Fact]
66+
public void SettingRequestHeaderEncodingSelecterThrowsArgumentNullException()
67+
{
68+
var options = new KestrelServerOptions();
69+
70+
var ex = Assert.Throws<ArgumentNullException>(() => options.RequestHeaderEncodingSelector = null);
71+
Assert.Equal("value", ex.ParamName);
72+
}
6373
}
6474
}

‎src/Servers/Kestrel/Core/test/KestrelServerTests.cs

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -245,22 +245,6 @@ public void StartWithMultipleTransportFactoriesDoesNotThrow()
245245
StartDummyApplication(server);
246246
}
247247

248-
[Fact]
249-
public void StartWithNullRequestHeaderEncodingSelectorThrows()
250-
{
251-
var kso = CreateServerOptions();
252-
kso.RequestHeaderEncodingSelector = null;
253-
254-
var testLogger = new TestApplicationErrorLogger { ThrowOnCriticalErrors = false };
255-
256-
using var server = CreateServer(kso, testLogger);
257-
258-
var ex = Assert.Throws<InvalidOperationException>(() => StartDummyApplication(server));
259-
Assert.Contains(nameof(KestrelServerOptions.RequestHeaderEncodingSelector), ex.Message);
260-
261-
Assert.Equal(1, testLogger.CriticalErrorsLogged);
262-
}
263-
264248
[Fact]
265249
public async Task StopAsyncCallsCompleteWhenFirstCallCompletes()
266250
{

0 commit comments

Comments
 (0)