Implement Header encoding selectors on SocketsHttpHandler #39468

MihaZupan · 2020-07-16T20:08:14Z

Contributes to #38711

Implemented:

namespace System.Net.Http
{
    public delegate Encoding? HeaderEncodingSelector<TContext>(string headerName, TContext context);

    public sealed class SocketsHttpHandler
    {
        public HeaderEncodingSelector<HttpRequestMessage>? RequestHeaderEncodingSelector { get; set; }
        public HeaderEncodingSelector<HttpRequestMessage>? ResponseHeaderEncodingSelector { get; set; }
    }
}

Part of the change is exposing overloads taking an Encoding in HPack and QPack encoders. HPack code is shared with aspnetcore, how are these changes propagated?

Loopback server changes:

Exposed the raw bytes for the received header value in LoopbackServer
Added support for Loopback servers to send header values with the specified encoding

ghost · 2020-07-16T20:08:17Z

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

Dotnet-GitSync-Bot · 2020-07-16T20:08:18Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Tratcher · 2020-07-16T21:10:58Z

Part of the change is exposing overloads taking an Encoding in HPack and QPack encoders. HPack code is shared with aspnetcore, how are these changes propagated?

See https://github.com/dotnet/runtime/blob/43b8caf5d455d7a2e32de13f813f4caf66d53340/src/libraries/Common/src/System/Net/Http/aspnetcore/ReadMe.SharedCode.md

halter73 · 2020-07-16T22:45:43Z

src/libraries/System.Net.Http/ref/System.Net.Http.cs

        [System.Diagnostics.CodeAnalysis.AllowNullAttribute]
        public System.Net.Security.SslClientAuthenticationOptions SslOptions { get { throw null; } set { } }
        public bool UseCookies { get { throw null; } set { } }
        public bool UseProxy { get { throw null; } set { } }
        protected override void Dispose(bool disposing) { }
        protected internal override System.Net.Http.HttpResponseMessage Send(System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { throw null; }
        protected internal override System.Threading.Tasks.Task<System.Net.Http.HttpResponseMessage> SendAsync(System.Net.Http.HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { throw null; }
+        public delegate System.Text.Encoding? HeaderEncodingSelector(string headerName, System.Net.Http.HttpRequestMessage request);


@Tratcher What do you think about switching Kestrel's RequestHeaderEncodingSelector to be a delegate System.Text.Encoding? HeaderEncodingSelector(string headerName) delegate?

It would be a new delegate type most likely defined in the Microsoft.AspNetCore.Server.Kestrel namespace since we don't have an HttpRequestMessage or really anything comparable in the early stages of request parsing. Having the same name as the System.Net.Http delegate might be too confusing though.

I originally went with Func<string, Encoding?> (that's at least what it will be once we support nullable reference types in that assembly) because we don't use delegates very much in configuration callbacks in ASP.NET Core. Thinking about it more though, that's because normally the argument is a specific options or context type. Since the argument is just a string, naming it feels more worthwhile. Increased consistency with System.Net.Http is an added bonus.

It would be a small usability improvement.

I'd avoid using the same name though, that gets awkward when you import both namespaces.

src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs

src/libraries/Common/src/System/Net/Http/aspnetcore/Http3/QPack/QPackEncoder.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

scalablecory

It looks pretty good but I'm up late and sleep deprived; I will do a 2nd pass when I have a clearer mind.

scalablecory

looks good, thanks. only trivial questions/suggestions.

scalablecory · 2020-07-28T05:49:45Z

src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs

@@ -466,13 +470,28 @@ public static bool EncodeStringLiteral(string value, Span<byte> destination, out
            if (destination.Length != 0)
            {
                destination[0] = 0; // TODO: Use Huffman encoding
-                if (IntegerEncoder.Encode(value.Length, 7, destination, out int integerLength))
+
+                int encodedStringLength = valueEncoding is null || ReferenceEquals(valueEncoding, Encoding.Latin1)


Do we have evidence that Latin1 will be used frequently or in perf-sensitive scenarios?

I would have expected people to largely use UTF-8.

Latin1 is the scenario required/requested to achieve compatibility with .Net Framework behavior (not throwing on non-ascii chars).

I expect the 99% use case for this to be UTF8 and Latin1, so I believe it is worth special-casing both, as keeping Latin1 optimizations shouldn't hurt UTF8 perf.

src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

scalablecory · 2020-07-28T06:08:15Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

+            int available = _writeBuffer.Length - offset;
+
+            // Fast-path Latin1 and UTF8 as the most common scenarios of non-default encoding
+            const int MaxUtf8BytesPerChar = 3;


UTF-8 uses a 4-byte max encoding, so this isn't immediately obviously correct. Can you use a comment similar to this one to make it clear?

runtime/src/libraries/System.Private.CoreLib/src/System/Text/UTF8Encoding.cs

Lines 49 to 56 in 6072e4d

/// <summary>

/// Transcoding to UTF-8 bytes from UTF-16 input chars will result in a maximum 3:1 expansion.

/// </summary>

/// <remarks>

/// Supplementary code points are expanded to UTF-8 from UTF-16 at a 4:2 ratio,

/// so 3:1 is still the correct value for maximum expansion.

/// </remarks>

private const int MaxUtf8BytesPerChar = 3;

@GrabYourPitchforks do we have some UTF-8 support code in /Common that we could place this constant in?

Simplified to just GetMaxByteCount, as it is a very fast method anyway.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs

MihaZupan · 2020-07-29T23:15:34Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-07-29T23:15:48Z

Azure Pipelines successfully started running 1 pipeline(s).

* Expose HeaderEncodingSelectors on SocketsHttpHandler * Implement header encoding selectors in SocketsHttpHandler * Add header encoding tests * Add summaries for new APIs * Use Stream.Write(byte[], int, int) overloads for framework compat * Add dummy API implementation to browser target * HPack/QPack fixes * Move HeaderEncodingSelector delegate to namespace, add TContext * Encoding fixes * Remove unused using * Simplify test * HttpConnection PR feedback * Simplify fast-path detection

MihaZupan added 4 commits July 14, 2020 17:58

Expose HeaderEncodingSelectors on SocketsHttpHandler

22ca8bb

Implement header encoding selectors in SocketsHttpHandler

2c425e4

Add header encoding tests

17b43ec

Add summaries for new APIs

43b8caf

MihaZupan added the area-System.Net.Http label Jul 16, 2020

MihaZupan added this to the 5.0.0 milestone Jul 16, 2020

MihaZupan requested review from a team July 16, 2020 20:08

MihaZupan self-assigned this Jul 16, 2020

Dotnet-GitSync-Bot added the new-api-needs-documentation label Jul 16, 2020

Tratcher requested a review from halter73 July 16, 2020 20:18

halter73 reviewed Jul 16, 2020

View reviewed changes

MihaZupan added 2 commits July 19, 2020 22:11

Use Stream.Write(byte[], int, int) overloads for framework compat

5a13ba0

Add dummy API implementation to browser target

202268f