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 |
|---|---|---|
|
|
@@ -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 (_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; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
X-Content-Type-OptionsandX-XSS-Protectionset; amongst others:Funnily
X-Content-Type-Optionsis the only one dotnet.microsoft.com does set: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
Linkheader rfc5988; is a good one for using for preloading of fonts prior to the css being parsed and html starting to render to increase webpage performance and reduce flash of unstyled content (FOUC) or flash of invisible text (FOIT) if you mark the text as unrenderable until the font is downloaded.e.g. Adding this header on the landing HTML page
link: <https://assets.ageofascent.net/fonts/teko-light-latin.woff2>; rel=preload; as=font; crossorigin=anonymous,<https://assets.ageofascent.net/fonts/pt_sans-web-regular.woff>; rel=preload; as=font; crossorigin=anonymousChrome/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
Linkis one of the missed headers I use; so fringe benefit in adding for me with this change 😉Its not adding it to the pre-reserved Kestrel headers; just to the
HeaderNamesconstants; as otherwise they look fugly in use:Though since you made me look; seems should add
"Referrer-Policy"also 🤔