Address PR feedback

Author: halter73

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

@@ -118,7 +118,6 @@ private void AppendContentLengthCustomEncoding(ReadOnlySpan<byte> value, Encodin
             _contentLength = parsed;
         }
 
-
         [MethodImpl(MethodImplOptions.NoInlining)]
         private void SetValueUnknown(string key, StringValues value)
         {

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

@@ -28,6 +28,7 @@ internal static partial class HttpUtilities
         private const ulong _http10VersionLong = 3471766442030158920; // GetAsciiStringAsLong("HTTP/1.0"); const results in better codegen
         private const ulong _http11VersionLong = 3543824036068086856; // GetAsciiStringAsLong("HTTP/1.1"); const results in better codegen
 
+        private static readonly UTF8EncodingSealed DefaultRequestHeaderEncoding = new UTF8EncodingSealed();
         private static readonly SpanAction<char, IntPtr> _getHeaderName = GetHeaderName;
         private static readonly SpanAction<char, IntPtr> _getAsciiStringNonNullCharacters = GetAsciiStringNonNullCharacters;
 
@@ -121,7 +122,7 @@ public static unsafe string GetAsciiStringNonNullCharacters(this ReadOnlySpan<by
         }
 
         public static string GetAsciiOrUTF8StringNonNullCharacters(this ReadOnlySpan<byte> span)
-            => StringUtilities.GetAsciiOrUTF8StringNonNullCharacters(span, KestrelServerOptions.DefaultRequestHeaderEncoding);
+            => StringUtilities.GetAsciiOrUTF8StringNonNullCharacters(span, DefaultRequestHeaderEncoding);
 
         private static unsafe void GetAsciiStringNonNullCharacters(Span<char> buffer, IntPtr state)
         {
@@ -140,14 +141,14 @@ public static string GetRequestHeaderString(this ReadOnlySpan<byte> span, string
         {
             if (ReferenceEquals(KestrelServerOptions.DefaultRequestHeaderEncodingSelector, encodingSelector))
             {
-                return span.GetAsciiOrUTF8StringNonNullCharacters(KestrelServerOptions.DefaultRequestHeaderEncoding);
+                return span.GetAsciiOrUTF8StringNonNullCharacters(DefaultRequestHeaderEncoding);
             }
 
             var encoding = encodingSelector(name);
 
             if (encoding is null)
             {
-                return span.GetAsciiOrUTF8StringNonNullCharacters(KestrelServerOptions.DefaultRequestHeaderEncoding);
+                return span.GetAsciiOrUTF8StringNonNullCharacters(DefaultRequestHeaderEncoding);
             }
 
             if (ReferenceEquals(encoding, Encoding.Latin1))
@@ -552,5 +553,13 @@ private static bool IsHex(char ch)
                 // Check if less than 6 representing chars 'a' - 'f'
                 || (uint)((ch | 32) - 'a') < 6u;
         }
+
+        // Allow for de-virtualization (see https://github.com/dotnet/coreclr/pull/9230)	
+        private sealed class UTF8EncodingSealed : UTF8Encoding	
+        {	
+            public UTF8EncodingSealed() : base(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true) { }	
+
+            public override byte[] GetPreamble() => Array.Empty<byte>();	
+        }
     }
 }

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

@@ -5,13 +5,11 @@
 using System.Collections.Generic;
 using System.IO.Pipelines;
 using System.Linq;
-using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Hosting.Server;
 using Microsoft.AspNetCore.Hosting.Server.Features;
-using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
@@ -374,11 +372,6 @@ private void ValidateOptions()
                 throw new InvalidOperationException(
                     CoreStrings.FormatMaxRequestBufferSmallerThanRequestHeaderBuffer(Options.Limits.MaxRequestBufferSize.Value, Options.Limits.MaxRequestHeadersTotalSize));
             }
-
-            if (Options.RequestHeaderEncodingSelector is null)
-            {
-                throw new InvalidOperationException($"{nameof(KestrelServerOptions)}.{nameof(KestrelServerOptions.RequestHeaderEncodingSelector)} must not be set to null.");
-            }
         }
 
         private static ConnectionDelegate EnforceConnectionLimit(ConnectionDelegate innerDelegate, long? connectionLimit, IKestrelTrace trace)

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

@@ -13,7 +13,6 @@
 using Microsoft.AspNetCore.Certificates.Generation;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
-using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.Https;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
@@ -26,11 +25,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core
     /// </summary>
     public class KestrelServerOptions
     {
-        // Internal to fast-path header decoding when RequestHeaderEncodingSelector is unchanged.
-        internal static readonly UTF8EncodingSealed DefaultRequestHeaderEncoding = new UTF8EncodingSealed();
-        internal static readonly Func<string, Encoding> DefaultRequestHeaderEncodingSelector = _ => DefaultRequestHeaderEncoding;
+        // internal to fast-path header decoding when RequestHeaderEncodingSelector is unchanged.
+        internal static readonly Func<string, Encoding?> DefaultRequestHeaderEncodingSelector = _ => null;
         internal static readonly Func<string, Encoding> DefaultLatin1RequestHeaderEncodingSelector = _ => Encoding.Latin1;
 
+        private Func<string, Encoding?> _requestHeaderEncodingSelector = DefaultRequestHeaderEncodingSelector;
+
         // 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.
         internal List<ListenOptions> CodeBackedListenOptions { get; } = new List<ListenOptions>();
         internal List<ListenOptions> ConfigurationBackedListenOptions { get; } = new List<ListenOptions>();
@@ -86,10 +86,11 @@ public class KestrelServerOptions
         /// Gets or sets a callback that returns the <see cref="Encoding"/> to decode the value for the specified request header name,
         /// or <see langword="null"/> to use the default <see cref="UTF8Encoding"/>.
         /// </summary>
-        /// <remarks>
-        /// Defaults to returning a <see langword="null"/> for all headers.
-        /// </remarks>
-        public Func<string, Encoding?> RequestHeaderEncodingSelector { get; set; } = DefaultRequestHeaderEncodingSelector;
+        public Func<string, Encoding?> RequestHeaderEncodingSelector
+        {
+            get => _requestHeaderEncodingSelector;
+            set => _requestHeaderEncodingSelector = value ?? throw new ArgumentNullException(nameof(value));
+        } 
 
         /// <summary>
         /// 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)
 
         internal Func<string, Encoding?> GetRequestHeaderEncodingSelector()
         {
-            if (ReferenceEquals(RequestHeaderEncodingSelector, DefaultRequestHeaderEncodingSelector) && Latin1RequestHeaders)
+            if (ReferenceEquals(_requestHeaderEncodingSelector, DefaultRequestHeaderEncodingSelector) && Latin1RequestHeaders)
             {
                 return DefaultLatin1RequestHeaderEncodingSelector;
             }
 
-            return RequestHeaderEncodingSelector
-                ?? throw new InvalidOperationException($"{nameof(KestrelServerOptions)}.{nameof(RequestHeaderEncodingSelector)} must not be set to null.");
+            return _requestHeaderEncodingSelector;
         }
 
         internal void ApplyEndpointDefaults(ListenOptions listenOptions)
@@ -256,7 +256,6 @@ private void EnsureDefaultCert()
         /// This will only reload endpoints defined in the "Endpoints" section of your <paramref name="config"/>. Endpoints defined in code will not be reloaded.
         /// </param>
         /// <returns>A <see cref="KestrelConfigurationLoader"/> for further endpoint configuration.</returns>
-
         public KestrelConfigurationLoader Configure(IConfiguration config, bool reloadOnChange)
         {
             var loader = new KestrelConfigurationLoader(this, config, reloadOnChange);

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

@@ -1,6 +1,7 @@
 // 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.Net;
 using Xunit;
 
@@ -60,5 +61,14 @@ public void CanCallListenAfterConfigure()
             // https://github.com/dotnet/aspnetcore/issues/21423
             options.ListenLocalhost(5000);
         }
+
+        [Fact]
+        public void SettingRequestHeaderEncodingSelecterThrowsArgumentNullException()
+        {
+            var options = new KestrelServerOptions();
+
+            var ex = Assert.Throws<ArgumentNullException>(() => options.RequestHeaderEncodingSelector = null);
+            Assert.Equal("value", ex.ParamName);
+        }
     }
 }

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

@@ -245,22 +245,6 @@ public void StartWithMultipleTransportFactoriesDoesNotThrow()
             StartDummyApplication(server);
         }
 
-        [Fact]
-        public void StartWithNullRequestHeaderEncodingSelectorThrows()
-        {
-            var kso = CreateServerOptions();
-            kso.RequestHeaderEncodingSelector = null;
-
-            var testLogger = new TestApplicationErrorLogger { ThrowOnCriticalErrors = false };
-
-            using var server = CreateServer(kso, testLogger);
-
-            var ex = Assert.Throws<InvalidOperationException>(() => StartDummyApplication(server));
-            Assert.Contains(nameof(KestrelServerOptions.RequestHeaderEncodingSelector), ex.Message);
-
-            Assert.Equal(1, testLogger.CriticalErrorsLogged);
-        }
-
         [Fact]
         public async Task StopAsyncCallsCompleteWhenFirstCallCompletes()
         {