Use interned Header Names for known headers not in the pre-allocated block #31311
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,7 @@ public static class HeaderNames | |
| public static readonly string Authorization = "Authorization"; | ||
|
|
||
| /// <summary>Gets the <c>baggage</c> HTTP header name.</summary> | ||
| public static readonly string Baggage = "Baggage"; | ||
|
|
||
| /// <summary>Gets the <c>Cache-Control</c> HTTP header name.</summary> | ||
| public static readonly string CacheControl = "Cache-Control"; | ||
|
|
@@ -166,6 +166,9 @@ public static class HeaderNames | |
| /// <summary>Gets the <c>Last-Modified</c> HTTP header name.</summary> | ||
| public static readonly string LastModified = "Last-Modified"; | ||
|
|
||
| /// <summary>Gets the <c>Link</c> HTTP header name.</summary> | ||
| public static readonly string Link = "Link"; | ||
|
|
||
| /// <summary>Gets the <c>Location</c> HTTP header name.</summary> | ||
| public static readonly string Location = "Location"; | ||
|
|
||
|
|
@@ -245,10 +248,10 @@ public static class HeaderNames | |
| public static readonly string Translate = "Translate"; | ||
|
|
||
| /// <summary>Gets the <c>traceparent</c> HTTP header name.</summary> | ||
| public static readonly string TraceParent = "TraceParent"; | ||
benaadams marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary>Gets the <c>tracestate</c> HTTP header name.</summary> | ||
| public static readonly string TraceState = "TraceState"; | ||
benaadams marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary>Gets the <c>Upgrade</c> HTTP header name.</summary> | ||
| public static readonly string Upgrade = "Upgrade"; | ||
|
|
@@ -274,10 +277,16 @@ public static class HeaderNames | |
| /// <summary>Gets the <c>WWW-Authenticate</c> HTTP header name.</summary> | ||
| public static readonly string WWWAuthenticate = "WWW-Authenticate"; | ||
|
|
||
| /// <summary>Gets the <c>X-Content-Type-Options</c> HTTP header name.</summary> | ||
| public static readonly string XContentTypeOptions = "X-Content-Type-Options"; | ||
|
There was a problem hiding this comment. Why are you adding Link, X-Content-Type-Options and X-XSS-Protection here? We don't intend this list to be exhaustive, we primarily add entries that are used directly by the framework. It's not clear why these were selected for special treatment in Kestrel. There was a problem hiding this comment.
www.webpagetest.org is one of the commonly used performance testing sites (along with Lighthouse based things for core web vitals e.g. web.dev/measure); and it will call you out; rightly or wrongly, for not having Funnily
vs
Just looks bad if you are explicitly called out for not setting them; but the Framework doesn't support them as a pre-defined headername when it has 90 other ones... The e.g. Adding this header on the landing HTML page
Chrome/Edge will then immediately start downloading the fonts; without yet having encountered the fonts in the parsing of the HTML (triggered by evaluating its css)
The browsers will also tell you off if you then don't use the preloaded font within the first few seconds of page load:
Anyway Its not adding it to the pre-reserved Kestrel headers; just to the
Though since you made me look; seems should add |
||
|
|
||
| /// <summary>Gets the <c>X-Frame-Options</c> HTTP header name.</summary> | ||
| public static readonly string XFrameOptions = "X-Frame-Options"; | ||
|
|
||
| /// <summary>Gets the <c>X-Requested-With</c> HTTP header name.</summary> | ||
| public static readonly string XRequestedWith = "X-Requested-With"; | ||
|
|
||
| /// <summary>Gets the <c>X-XSS-Protection</c> HTTP header name.</summary> | ||
| public static readonly string XXssProtection = "X-XSS-Protection"; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,16 +12,17 @@ | |
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; | ||
| using Microsoft.Extensions.Primitives; | ||
| using Microsoft.Net.Http.Headers; | ||
|
|
||
| namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http | ||
| { | ||
| internal abstract partial class HttpHeaders : IHeaderDictionary | ||
| { | ||
| protected long _bits = 0; | ||
| protected long? _contentLength; | ||
| protected bool _isReadOnly; | ||
| protected Dictionary<string, StringValues>? MaybeUnknown; | ||
| protected Dictionary<string, StringValues> Unknown => MaybeUnknown ??= new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| public long? ContentLength | ||
| { | ||
|
|
@@ -40,6 +41,8 @@ public long? ContentLength | |
| } | ||
| } | ||
|
|
||
| public abstract StringValues HeaderConnection { get; set; } | ||
|
|
||
| StringValues IHeaderDictionary.this[string key] | ||
| { | ||
| get | ||
|
|
@@ -126,6 +129,18 @@ public void Reset() | |
| ClearFast(); | ||
| } | ||
|
|
||
| protected static string GetInternedHeaderName(string name) | ||
|
There was a problem hiding this comment. This method looks like it will be used to set unknown headers. Aren't most of the headers in this collection known? They will never be used because generated code will handle setting them, right? There was a problem hiding this comment. There are 93 header names in However yes, if the header is in the set for Any headers outside the set in There was a problem hiding this comment. Ok! This two phases of hash lookups (intern hashset then unknown dictionary) makes me wonder whether it makes sense to write a custom collection in the future. As well as optimizing interning known keys, there is probably an opportunity to optimize memory usage. Most requests have few unknown headers, so O(n) lookups wouldn't matter. e.g. an adaptive collection that can switch between There was a problem hiding this comment.
Is a question on whether to extend the values in Though response set headers would likely be less important than request set headers; assuming they are being set by a single assembly so already an interned value (beyond developer convenience); though a pass-through name where Kestrel was acting as a proxy would still be valuable. However that's probably a separate issue... There was a problem hiding this comment. Added issue for other headers #31374 |
||
| { | ||
| // Some headers can be very long lived; for example those on a WebSocket connection | ||
| // so we exchange these for the preallocated strings predefined in HeaderNames | ||
| if (s_internedHeaderNames.TryGetValue(name, out var internedName)) | ||
| { | ||
| return internedName; | ||
| } | ||
|
|
||
| return name; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| protected static StringValues AppendValue(StringValues existing, string append) | ||
| { | ||
|
|
@@ -276,7 +291,12 @@ public static void ValidateHeaderNameCharacters(string headerCharacters) | |
| } | ||
| } | ||
|
|
||
| private readonly static string KeepAlive = "keep-alive"; | ||
| private readonly static StringValues ConnectionValueKeepAlive = KeepAlive; | ||
| private readonly static StringValues ConnectionValueClose = "close"; | ||
| private readonly static StringValues ConnectionValueUpgrade = HeaderNames.Upgrade; | ||
|
|
||
| public static ConnectionOptions ParseConnection(HttpHeaders headers) | ||
| { | ||
| // Keep-alive | ||
| const ulong lowerCaseKeep = 0x0000_0020_0020_0020; // Don't lowercase hyphen | ||
|
|
@@ -289,9 +309,27 @@ public static ConnectionOptions ParseConnection(StringValues connection) | |
| // Close | ||
| const ulong closeEnd = 0x0065_0073_006f_006c; // 4 chars "lose" | ||
|
|
||
| var connection = headers.HeaderConnection; | ||
| var connectionCount = connection.Count; | ||
| if (connectionCount == 0) | ||
| { | ||
| return ConnectionOptions.None; | ||
| } | ||
|
|
||
| var connectionOptions = ConnectionOptions.None; | ||
|
|
||
| if (connectionCount == 1) | ||
| { | ||
| // "keep-alive" is the only value that will be repeated over | ||
| // many requests on the same connection; on the first request | ||
| // we will have switched it for the readonly static value; | ||
| // so we can ptentially short-circuit parsing and use ReferenceEquals. | ||
| if (ReferenceEquals(connection.ToString(), KeepAlive)) | ||
| { | ||
| return ConnectionOptions.KeepAlive; | ||
| } | ||
| } | ||
|
|
||
| for (var i = 0; i < connectionCount; i++) | ||
| { | ||
| var value = connection[i].AsSpan(); | ||
|
|
@@ -420,6 +458,21 @@ public static ConnectionOptions ParseConnection(StringValues connection) | |
| } | ||
| } | ||
|
|
||
| // If Connection is a single value, switch it for the interned value | ||
| // in case the connection is long lived | ||
| if (connectionOptions == ConnectionOptions.Upgrade) | ||
| { | ||
| headers.HeaderConnection = ConnectionValueUpgrade; | ||
| } | ||
| else if (connectionOptions == ConnectionOptions.KeepAlive) | ||
| { | ||
| headers.HeaderConnection = ConnectionValueKeepAlive; | ||
| } | ||
| else if (connectionOptions == ConnectionOptions.Close) | ||
| { | ||
| headers.HeaderConnection = ConnectionValueClose; | ||
| } | ||
|
|
||
| return connectionOptions; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.