Skip to content

Implement Header encoding selectors on SocketsHttpHandler#39468

Merged
MihaZupan merged 15 commits into
dotnet:masterfrom
MihaZupan:header-encoding-socketshttp
Jul 30, 2020
Merged

Implement Header encoding selectors on SocketsHttpHandler#39468
MihaZupan merged 15 commits into
dotnet:masterfrom
MihaZupan:header-encoding-socketshttp

Conversation

@MihaZupan

@MihaZupan MihaZupan commented Jul 16, 2020

Copy link
Copy Markdown
Member

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

@MihaZupan MihaZupan added this to the 5.0.0 milestone Jul 16, 2020
@MihaZupan MihaZupan requested review from a team July 16, 2020 20:08
@MihaZupan MihaZupan self-assigned this Jul 16, 2020
@ghost

ghost commented Jul 16, 2020

Copy link
Copy Markdown

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

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

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

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);

@halter73 halter73 Jul 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a small usability improvement.

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

Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http3/QPack/QPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http3/QPack/QPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http3/QPack/QPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http3/QPack/QPackEncoder.cs Outdated
Comment thread src/libraries/Common/src/System/Net/Http/aspnetcore/Http3/QPack/QPackEncoder.cs Outdated

@scalablecory scalablecory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 scalablecory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks. only trivial questions/suggestions.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs
int available = _writeBuffer.Length - offset;

// Fast-path Latin1 and UTF8 as the most common scenarios of non-default encoding
const int MaxUtf8BytesPerChar = 3;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

/// <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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MihaZupan

Copy link
Copy Markdown
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan MihaZupan merged commit 02e872e into dotnet:master Jul 30, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
* 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
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants