Add HTTP/3 #1294

scalablecory · 2020-01-04T13:26:19Z

This adds HTTP/3 (draft 24) support to SocketsHttpHandler.

Marked NO MERGE for now because it a) depends on some QUIC APIs that don't exist yet, and b) due to that hasn't been tested against a live server.

This adds the APIs in #1293.

There are some minor optimization opportunities (mostly in removing some async allocations) that will be investigated once things are in a state that we can benchmark.

Some of the initial H3 code from Kestrel is shared and thus included in this PR. @jkotalik this code has had some changes mostly to support Span, but also some optimizations.

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

jkotalik · 2020-01-05T02:09:42Z

src/libraries/Common/src/System/Net/Http/Http2/Hpack/IntegerEncoder.cs

@@ -8,6 +8,11 @@ namespace System.Net.Http.HPack
 {
    internal static class IntegerEncoder
    {
+        /// <summary>


Move IntegerEncoder out of Http2 (as it's used by Http3)?

Need to figure out the sharing script for this. Maybe just merge the http/2 and http/3 into a single shared directory structure.

It looks like this is the only type that's shared between Http2 and Http3? If so then having Http3 reference the Http2 class/file should be fine. I'd like to keep the other files separate for clarity.

How about this new dir structure? No need to duplicate the scripts.

src/libraries/Common/src/System/Net/Http/AspShared

/Http2

/Http3

Copy scripts, readme, etc.

Yep, that's in line with what I was thinking.

jkotalik · 2020-01-05T02:10:49Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3ErrorCode.cs

@@ -0,0 +1,105 @@
+// Copyright (c) .NET Foundation. All rights reserved.


Probably swap these copyrights to MIT as that's what is being used in corefx.

I agree, but I'm not sure enough of the technicalities to know if it's legal for us to change it. Our strategy with HPACK was to keep the original license from ASP.NET. @terrajobst any opinions?

cc: @richlander

Go ahead ... make the license change.

jkotalik · 2020-01-05T02:11:48Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3Frame.cs

+{
+    internal static partial class Http3Frame
+    {
+        public static bool TryReadIntegerPair(ReadOnlySpan<byte> buffer, out long a, out long b, out int bytesRead)


Suggested change

public static bool TryReadIntegerPair(ReadOnlySpan<byte> buffer, out long a, out long b, out int bytesRead)

public static bool TryReadVariableLengthIntegerPair(ReadOnlySpan<byte> buffer, out long a, out long b, out int bytesRead)

as the out params are longs, either we should consistently say VariableLengthInteger or Long instead of Integer.

TryReadVLIPair? 😁
VariableLengthInteger is the spec term, but it's a bit of a mouthful.

I feel like the current name is fine, but if others express a strong opinion I'm not opposed to changes.

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3Frame.cs

src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs

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

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

jkotalik · 2020-01-05T02:37:41Z

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

+                    }
+
+                    QuicStream stream = await streamTask.ConfigureAwait(false);
+                    _ = ProcessServerStreamAsync(stream);


I'd keep this task around and await it in some cleanup code.

There's no need here; the process is cleaned up when the connection is disposed, and errors are only observed through SendAsync(). Will add a comment though.

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

gfoidl · 2020-01-05T17:07:10Z

src/libraries/Common/src/System/Net/Http/Http3/QPack/QPackDecoder.cs

+            }
+            catch (HuffmanDecodingException ex)
+            {
+                throw new QPackDecodingException("TODO sync with corefx" /*CoreStrings.QPackHuffmanError, */, ex);


Suggested change

throw new QPackDecodingException("TODO sync with corefx" /*CoreStrings.QPackHuffmanError, */, ex);

throw new QPackDecodingException("TODO sync with runtime/libraries" /*CoreStrings.QPackHuffmanError, */, ex);

?

Ultimately these strings need to be put into resources.

I have shared resources mostly working in the Http2 code.

Let's leave this as a follow up item.

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

src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs

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

gfoidl · 2020-01-05T18:13:24Z

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

+                // writes more bytes than was precomputed (this latter case is probably an error -- TODO).
+                // In this case, each write results in a data frame. TODO: optimize this to avoid small frame sizes.
+
+                byte[] dataFrame = _dataFrame;


Suggested change

byte[] dataFrame = _dataFrame;

byte[] dataFrame = _dataFrame ??= new byte[9];

Saves some C# code when the following if is removed. More or less a matter of taste.

I'm fine with it now; will update if there are strong opinions.

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

jkotalik · 2020-01-06T15:40:19Z

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

+                : WaitThenSendAsync(waitTask, request, cancellationToken);
+        }
+
+        private async Task<HttpResponseMessage> WaitThenSendAsync(Task taskToWaitFor, HttpRequestMessage request, CancellationToken cancellationToken)


nit: ValueTask all the things?

For now, only the things I expect to finish synchronously.

In the future, depending on the outcome of @stephentoub's ValueTask caching patch, maybe ValueTask all the things.

I think it's reasonable to use ValueTask<T> for any of these that are internal/private, as long as we're only ever awaiting them at the call sites. That will help us to better evaluate the ValueTask opt-in change I made.

@stephentoub I'm open to it, but how will it help us evaluate things when there's no baseline to compare to?

The comparison I'm referring to is when lots of stuff uses ValueTask<T>, and we flip the switch to enable automatic pooling (if very little is using ValueTask<T> in our code, then flipping that switch won't produce any meaningful results). If the method completes synchronously, then using ValueTask<T> was a good idea, and if it completes asynchronously, then the overhead compared to the allocation/state management/etc. is minimal, so I'm not particularly concerned with a comparison about HTTP/3's implementation with ValueTask<T> vs Task<T> here.

Thanks. I'll leave this for a post-merge update.

jkotalik · 2020-01-06T15:41:18Z

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

+                Abort(ex);
+            }
+
+        retryRequest:


nit: scrap the goto 😄

It's more of a personal opinion. I think it generally makes code less readable and usually can be fixed via refactoring. We avoid them in AspNetCore, not sure if it varies in the runtime repo.

I'll take another look to see if they can reasonably be removed without a size/perf penalty.

Does the goto mess up the lock statement? I'd rather not find out.
A local method or just inlining the code seems more appropriate here.

Does the goto mess up the lock statement?

C# doesn't let you create irreducible control-flow graphs with goto; so no. (i.e. you can't jump into a different scope).

It outputs a leave.s il instruction inside a lock, executing the finally first, undoing the lock

The leave.s instruction is similar to the br instruction, but it can be used to exit a try, filter, or catch block whereas the ordinary branch instructions can only be used in such a block to > transfer control within it. The leave.s instruction empties the evaluation stack and ensures that the appropriate surrounding finally blocks are executed.

tl;dr C# won't compile if your goto use is unsound (unlike other languages)

Aside, the C# compiler on the other hand... does create irreducible control-flow graphs in il, in certain circumstances dotnet/roslyn#39814

Leaving this as a post-merge TODO. I'm not worried about the goto specifically but I'm going to go over the entire locking strategy, which was made a bit coarse to start with, and goto might naturally go away as part of that -- either way, once I get to this method I'll see if it's reasonable to get rid of it.

jkotalik · 2020-01-06T15:45:11Z

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

+                _requestStreamsRemaining += (newMaximumStreamCount - _maximumRequestStreams);
+                _maximumRequestStreams = newMaximumStreamCount;
+
+                while (_requestStreamsRemaining != 0 && _waitingRequests.TryDequeue(out TaskCompletionSourceWithCancellation<bool> tcs))


I'm a bit confused by the _waitingRequests queue here. This looks like it is draining all pending calls to SendAsync. Why do we need to do this?

This is to respond to new MAX_STREAMS events from QUIC. _waitingRequests are any requests waiting for a new stream to open up.

I think you need another place where you dequeue when a request is finished. Ex: max request stream count is 100, someone makes 101 requests, so one request is waiting. When a request finishes, it needs to call TryDequeue to start the next request, correct?

MAX_STREAMS is a cumulative maximum, not a maximum concurrent streams:

Note that these frames (and the corresponding transport parameters) do not describe the number of streams that can be opened concurrently. The limit includes streams that have been closed as well as those that are open.
https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#section-19.11-8

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

jkotalik · 2020-01-06T15:54:05Z

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

+
+                while (!VariableLengthIntegerHelper.TryRead(buffer.AvailableSpan, out lastStreamId, out bytesRead))
+                {
+                    buffer.EnsureAvailableSpace(1);


This seems inefficient. This is only growing the buffer by one each time, which causes TryRead and ReadAsync to be called multiple times if we are reading a large integer.

It looks like ArrayBuffer grows by doubling even if only 1 byte is needed

Okay not as bad as I initially thought. Maybe something should be renamed to make it more clear that we are increasing the space by more than just 1 (at least double).

I still think this pattern of growing the buffer is prone to smaller reads though.

Current strategy is to: read in just enough to parse headers in one I/O, and then let user control read pattern through Content.

Once we get implementation finished, we'll want to tune this. It's not clear what buffer size will be optimal just yet, but it will be trivial to change when we're ready.

jkotalik · 2020-01-06T16:11:58Z

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

+                while (!Http3Frame.TryReadIntegerPair(_buffer.ActiveSpan, out frameType, out payloadLength, out bytesRead))
+                {
+                    _buffer.EnsureAvailableSpace(2);
+                    bytesRead = await _stream.ReadAsync(_buffer.AvailableMemory, cancellationToken).ConfigureAwait(false);


These small reads don't seem like they would be good for perf. Do you think it may be better to preemptively allocate some amount in the buffer s.t. future calls that use the payload length already have it?

AvailableMemory will typically be much larger here. 2 is just the minimum that we need for two integers.

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

jkotalik · 2020-01-06T16:17:01Z

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

+        private int ReadResponseContent(HttpResponseMessage response, Span<byte> buffer)
+        {
+            // This should only be called by our read stream, and our local _response should be nulled out by now.
+            Debug.Assert(_response == null);


I see this called from Read(), why does response need to be null?

Once the response headers are read, HttpRequestStream._response will be nulled out to avoid circular references. By the time we're reading content, this should already have happened. I'll see about improving the comment.

I see, what circular references are you worried about?

stream -> response -> content -> stream

jkotalik · 2020-01-06T16:19:24Z

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

+                while (buffer.Length != 0)
+                {
+                    // Sync over async here -- QUIC implementation does it per-I/O already; this is at least more coarse-grained.
+                    if (_dataPayloadRemaining <= 0 && !ReadNextDataFrameAsync(response, CancellationToken.None).GetAwaiter().GetResult())


This will probably need to change eventually to no do sync over async.

Yea, I had same thought -- leaving as a TODO until we firm up the behavior of QuicStream.

jkotalik · 2020-01-06T16:20:38Z

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

+            }
+        }
+
+        private async ValueTask<int> ReadResponseContentAsync(HttpResponseMessage response, Memory<byte> buffer, CancellationToken cancellationToken)


Codeshare between ReadResponseContent/ReadResponseContentAsync

Maybe I'm mistaken, but the only difference I see is that ReadNextDataFrameAsync is called sync in ReadResponseContent and async in ReadResponseContentAsync.

At a minimum, you can share the logic to calculate the totalBytesRead and update the buffers.

Stream.Read vs Stream.ReadAsync as well. I can share the exception handling; I'll do that.

jkotalik · 2020-01-06T16:23:11Z

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

+        public void Trace(string message, [CallerMemberName] string memberName = null) =>
+            _connection.Trace(_stream.StreamId, message, memberName);
+
+        private sealed class Http3ReadStream : HttpBaseStream


Can Http3RequestStream derive from HttpBaseStream s.t. you can avoid allocating an extra object per request?

Possibly, but it gets weird as the users can dispose the stream. I will leave this for future TODO to look into.

jkotalik · 2020-01-06T16:24:37Z

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

+            }
+        }
+
+        private sealed class Http3WriteStream : HttpBaseStream


Same here, can Http3WriteStream be removed and part of Http3RequestStream?

Tratcher · 2020-01-06T18:03:40Z

src/libraries/Common/src/System/Net/Http/Http2/Hpack/H2StaticTable.cs

@@ -7,7 +7,7 @@

 namespace System.Net.Http.HPack
 {
-    internal static class StaticTable
+    internal static class H2StaticTable


More accurately these are the HPack and QPack static tables, not the H2 and H2 tables. How about calling them HPack.StaticTable in the few places both namespaces are in scope?

It's easier to disambiguate them this way, without having to see what using is in effect. I'd like them to have separate names.

Maybe rename to HPackStaticTable and QPackStaticTable?

Tratcher · 2020-01-06T18:12:44Z

src/libraries/Common/src/System/Net/Http/Http2/Hpack/IntegerEncoder.cs

@@ -8,6 +8,11 @@ namespace System.Net.Http.HPack
 {
    internal static class IntegerEncoder
    {
+        /// <summary>


It looks like this is the only type that's shared between Http2 and Http3? If so then having Http3 reference the Http2 class/file should be fine. I'd like to keep the other files separate for clarity.

How about this new dir structure? No need to duplicate the scripts.

src/libraries/Common/src/System/Net/Http/AspShared

/Http2

/Http3

Copy scripts, readme, etc.

src/libraries/Common/src/System/Net/Http/Http2/IHttpHeadersHandler.cs

Tratcher · 2020-01-06T18:17:47Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3ErrorCode.cs

+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+#if KESTREL
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3


We don't need this kestrel namespace, the internal shared types can be in System.Net.Http. Only IHttpHeadersHandler needed to preserve the namespace because it's public.

Yeah go ahead and make the changes here, we will react in Kestrel to the namespace changes.

Only IHttpHeadersHandler needed to preserve the namespace because it's public.

We're adding new methods to the interface. That's similarly breaking, no? I thought the purpose of the pubternals approach was that API changes were permitted.

I think we can change IHttpHeadersHandler to be in the System.Net.Http namespace, but IIRC we use IHttpHeadersHandler in our benchmarks repo. cc @sebastienros we may update a namespace that we'd need to react to in benchmarks, that okay?

Always ok if you apply the change in the benchmarks too ;)

IHttpHeadersHandler is an internal type in System.Net.Http.dll, it doesn't matter what the namespace is there.

IHttpHeadersHandler is a public (pubternal) type in Kestrel. Yes we can make API changes to pubternal types, that's not the question here. We should not use the System.Net.Http namespace for public types in Kestrel.

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3ErrorCode.cs

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3Frame.cs

Tratcher · 2020-01-06T18:40:50Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3Frame.cs

+{
+    internal static partial class Http3Frame
+    {
+        public static bool TryReadIntegerPair(ReadOnlySpan<byte> buffer, out long a, out long b, out int bytesRead)


TryReadVLIPair? 😁
VariableLengthInteger is the spec term, but it's a bit of a mouthful.

Tratcher · 2020-01-06T18:45:56Z

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

+            return integerValue;
+        }
+
+        static bool TryDecodeHttpInteger(ReadOnlySpan<byte> buffer, out long value, out int bytesRead)


Duplicated from VariableLengthIntegerHelper?

Intentional -- our loopback code does not share product code.

eek. That seems like an odd decision to me. What are the main reasons for not sharing "product code". Can you do internals visible to?

What are the main reasons for not sharing "product code".

Two implementations of something make it easier to find bugs, and the test implementation often sacrifices perf for flexibility or simplicity.

Can you do internals visible to?

If we did anything it would be to include the source directly; InternalsVisibleTo is a hard no per various discussions on its goodness.

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

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

Tratcher

Incremental review

src/libraries/Common/src/System/Net/Http/Http3/Http3SettingType.cs

src/libraries/Common/src/System/Net/Http/Http3/Http3StreamType.cs

src/libraries/Common/src/System/Net/Http/Http3/QPack/H3StaticTable.cs

src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderValue.cs

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

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

stephentoub · 2020-01-17T14:18:33Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3ErrorCode.cs

+        /// The requested operation cannot be served over HTTP/3. The peer should retry over HTTP/1.1.
+        /// </summary>
+        VersionFallback = 0x110,
+    }


I'm disappointed there's no "EnhanceYourCalm" 😄

Alas, it was renamed to H3_EXCESSIVE_LOAD. 😢

stephentoub · 2020-01-17T14:21:11Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+{
+    /// <summary>
+    /// Variable length integer encoding and decoding methods. Based on https://tools.ietf.org/html/draft-ietf-quic-transport-24#section-16.
+    /// Either will take up 1, 2, 4, or 8 bytes.


Nit: "Either"?

stephentoub · 2020-01-17T14:24:53Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+        public const uint OneByteLimit = 64;
+        private const uint TwoByteLimit = 16383;
+        private const uint FourByteLimit = 1073741823;
+        private const long EightByteLimit = 4611686018427387903L; // 2^62-1


Rather than writing out the number, could we instead write this as something like (long)((1UL << 62) - 1)? Same goes for the others. The comment at that point isn't necessary, and it's clear how the value is derived rather than having a magic value.

In the spec it's listed as the entire integer, so it was easier to copy like that. https://tools.ietf.org/html/draft-ietf-quic-transport-24#section-16

stephentoub · 2020-01-17T14:25:49Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+        private const byte LengthOneByte = 0x00;
+        private const byte LengthTwoByte = 0x40;
+        private const byte LengthFourByte = 0x80;
+        private const byte LengthEightByte = 0xC0;


Nit: LengthTwoBytes, LengthFourBytes, etc.?

stephentoub · 2020-01-17T14:29:10Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+                            _ => 8 // LengthEightByte
+                    };
+
+                    Span<byte> temp = (stackalloc byte[8])[..length];


Why do we need to slice it?

(stackalloc byte[8])[..length] has marginally better codegen than stackalloc byte[length].

I wasn't questioning that part, but rather why does it need to be perfectly sized at all. Why not just stackalloc byte[8]?

Oh. SequenceReader.TryCopyTo reads in the full buffer or fails, so 8 would cause EOF scenario to fail.

I don't understand. temp here is the target of TryCopyTo, not the source. What am I missing?

It's about L121 with TryRead(temp, out ...)

What about it? That TryRead is taking a span, not a reader, and it's not using a reader internally.

Ah, with the current implementation of TryRead it will work with any large enough size. So you're correct, slicing to actual size isn't needed.

(I believe to remember there were a version of TryRead expecting the actual size to work -- but irrelevant right now.)

Slice is required here because of reader.TryCopyTo:

Our stream has reach EOF and the reader has exactly 2 bytes left, containing a 2-byte integer.

reader.TryCopyTo -- which succeeds only if it can fill the entire span -- fails because there are not 8 bytes available.

Upstream we see EOF but VariableLengthIntegerHelper.TryRead fails, so it throws something indicating an incomplete stream.

which succeeds only if it can fill the entire buffer

Ugh. That is not an intuitive design. That's not how any other "CopyTo" I'm aware of behaves.

It got @GrabYourPitchforks too. Somehow nobody caught it during API review.

stephentoub · 2020-01-17T14:31:23Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+            }
+        }
+
+        public static long GetInteger(in ReadOnlySequence<byte> buffer, out SequencePosition consumed, out SequencePosition examined)


Does using in for ReadOnlySequence<byte> measurably help? On Linux, too?

I would need to dig up the benchmark, but my general policy with in is based on the size of the struct. ROS is a relatively large struct, so my policy has to pass it via in.

@benaadams do you happen to have concrete numbers on passing ROS via in?

stephentoub · 2020-01-17T14:31:58Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+            if (TryRead(ref reader, out long value))
+            {
+                consumed = examined = buffer.GetPosition(reader.Consumed);
+                return (long)value;


value is already declared as long. Why are we casting to long?

Leftover from me trying to use proper unsigned types. Will remove.

stephentoub · 2020-01-17T14:38:23Z

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs

+
+            if (longToEncode < OneByteLimit)
+            {
+                if (!buffer.IsEmpty)


Nit: in other places like https://github.com/dotnet/runtime/pull/1294/files#diff-fb1bdc75c84ead7a94a9c379a52f6230R46, we've used buffer.Length != 0. It'd be nice to be consistent one way or the other. Personally I prefer the Length comparison in cases like this where we're writing out, simply because an "empty" check makes me think we're trying to read something from it, as we're checking to see if anything is there.

@gfoidl IIRC you mentioned that checking buffer.Length != 0 is faster than buffer.IsEmpty. Is that correct?

It results in the same codegen -- so no perf-difference. My suggestion was about readability / expressiveness, and IMHO buffer.IsEmpty does a good job therefore.

Edit: I'm also fine with @stephentoub rationale for the length-check for the "output side". We should be consistent accross the board. When the rule is "input -> IsEmpty, output -> length-check" then it's good and clear.

stephentoub · 2020-01-17T14:43:20Z

src/libraries/Common/src/System/Net/Http/Http3/QPack/DynamicTable.cs

+        {
+        }
+
+        // TODO


Is there an issue link for all of these TODOs?

I wasn't sure which repo to file issues in (these are ported from aspnetcore). I think we can start filing issues in the runtime repo now :)

stephentoub · 2020-01-17T14:43:42Z

src/libraries/Common/src/System/Net/Http/Http3/QPack/H3StaticTable.cs

+{
+    internal class H3StaticTable
+    {
+        private static readonly H3StaticTable _instance = new H3StaticTable();


This whole class needs to be made static. Going to leave a comment and do post-merge.

jkotalik · 2020-01-17T18:26:01Z

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

+        private HttpResponseMessage _response;
+
+        // Header decoding.
+        private QPackDecoder _headerDecoder;


We should probably sync about how the QPackDecoder references the DynamicTable. From what I can tell, the Dynamic table will be per connection, so it may be tricky to get that all correct.

I'm leaning towards removing DynamicTable from H3 completely. It's complex and not MVP -- we can add later once everything else is working.

If you can confirm we can remove it entirely on both the client and server, then let's do it.

jkotalik · 2020-01-17T18:30:40Z

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

+                        }
+                        break;
+                    default:
+                        Debug.Fail($"Recieved unexpected frame type {frameType}.");


I don't think this should be a debug fail.

ReadFrameEnvelopeAsync does its own checks; the cases here are the only ones it should see.

jkotalik · 2020-01-17T18:31:56Z

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

+            }
+        }
+
+        private void BufferHeaders(HttpRequestMessage request)


nit: CommitHeaders? We use this terminology when we write the headers to a buffer and don't flush them.

jkotalik · 2020-01-17T18:32:39Z

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

+            }
+        }
+
+        private void BufferIndexedHeader(int index)


Same with these names: Buffer => Commit.

jkotalik · 2020-01-17T18:33:16Z

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

+                    throw new Http3ConnectionException(Http3ErrorCode.ProtocolError);
+                }
+
+                int statusCode = staticIndex switch


I thought we had a table for this in H3StaticTable?

I believe we had the reverse table, but I could be wrong.

jkotalik · 2020-01-17T18:50:45Z

Alright, I did a second round of review and this is making great progress.

We should discuss what our timeline is for merging this. Ideally, I'd like to start reacting to this in Kestrel somewhat soon.

Tratcher

This PR has gotten too large to effectively review. At 7K lines it's literally lagging my browser. What can be done to reduce the scope so we can get the core reviewed and merged before further iterating? E.g. can we skip all of the QPack optimizations?

Tratcher · 2020-01-10T18:10:17Z

src/libraries/Common/src/System/Net/Http/Http2/IHttpHeadersHandler.cs

@@ -4,7 +4,6 @@

 #if KESTREL
 using System;
-


Tratcher · 2020-01-10T18:12:40Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3ErrorCode.cs

+    internal enum Http3ErrorCode : uint
+    {
+        /// <summary>
+        /// HTTP_NO_ERROR (0x100):


In draft 24 (https://tools.ietf.org/html/draft-ietf-quic-http-24#section-8.1) these were renamed to H3_NO_ERROR

Tratcher · 2020-01-10T18:25:12Z

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

+        public string Origin;
+        public string AltSvc;
+
+        public AltSvcFrame(string origin, string altSvc, int streamId)


Document the expected encoding state here, these must be respectively pre-encoded or it will affect the lengths.

Tratcher · 2020-01-10T18:35:56Z

src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs

+                    else
+                    {
+                        // RFC makes it unclear what to do if a duplicate parameter is found. For now, take the minimum.
+                        maxAge = Math.Min(maxAge.GetValueOrDefault(), maxAgeTmp);


Suggested change

maxAge = Math.Min(maxAge.GetValueOrDefault(), maxAgeTmp);

maxAge = Math.Min(maxAge.Value, maxAgeTmp);

You already checked for null. Default would never work here anyways.

This is an optimization. Value is implemented as HasValue ? GetValueOrDefault() : throw, and JIT isn't yet smart enough to remove the dead branch.

src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs

Tratcher · 2020-01-10T18:57:22Z

src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs

+
+            int lowerCase = ch | 0x20;
+
+            if ((uint)(lowerCase - 'a') <= 'f' - 'a') // lowerCase >= 'a' && lowerCase <= 'f'


Upper case here only too.

Tratcher · 2020-01-10T22:53:16Z

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

+        private readonly Dictionary<long, Http3RequestStream> _activeRequests = new Dictionary<long, Http3RequestStream>();
+
+        // Set when GOAWAY is being processed, when aborting, or when disposing.
+        private long _lastProcessedStreamId = -1;


Style: use long? and HasValue rather than a sentinel value -1.

I'm fine with this as it's not a public API and saves some space. Our HTTP/2 code does the same.

Tratcher · 2020-01-10T23:00:05Z

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

+                Abort(ex);
+            }
+
+        retryRequest:


Does the goto mess up the lock statement? I'd rather not find out.
A local method or just inlining the code seems more appropriate here.

Tratcher · 2020-01-17T21:52:30Z

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

+            OnHeader(staticIndex: null, descriptor, staticValue: default, literalValue: value);
+        }
+
+        void IHttpHeadersHandler.OnStaticIndexedHeader(int index)


This is a useful optimization for known fields, but it's leaking H/Qpack implementation details into the request streams. I think there's a way to do this without leaking, and in a way that gives us more opportunities for optimizations.

Define a KnownHeaderId enum for any header names we care about. This aligns closely with the KnownHeader(s) classes you already have. This doesn't need to be restricted to the ones defined in H/Qpack's static tables, we could also use it for dynamic compression optimizations later.

Have H/Qpack decoders map static values to the KnownHeaderId enum. The decoders should also handle the known values mapping internally. This abstracts out the compression details early and allows for more unified code paths later in the flow.

Replace IHttpHeadersHandler.OnStaticIndexedHeader(int index) and IHttpHeadersHandler.OnStaticIndexedHeader(int index, ReadOnlySpan value) with IHttpHeadersHandler.OnKnownHeader(KnownHeaderId knownHeader, ReadOnlySpan value). This implementation can share most code across H2 and H3 since it uses unified ids.

This means HeaderDescriptor doesn't have to know about H/QPack implementation details, only a KnownHeaderId enum value and a unified lookup table for KnownHeaders.

Lets chat about this next week. I'm not as concerned about leaking some of QPack into H3 as they are tightly intertwined already, but I'd like to hear more about what other optimizations you feel we can get out of this abstraction.

Initially I flagged this as a testing/maintenance issue. QPack can't be reviewed or tested in isolation because this chunk of it has been implemented directly inside the H3RequestStream.

Looking over this again I see a bigger problem. IHttpHeadersHandler is the boundary between the shared code (QPack) and the non-shared code (Http3RequestStream). All of the QPack logic you implement inside Http3RequestStream is code we have to duplicate in Kestrel. We should be able to share all of the QPack logic but to do so it needs to be factored appropriately.

Follow-up on offline convo:

There's still concern that the abstraction as proposed will not provide real maintainability benefit and will be inefficient due to extra LUTs. We're going to give it a look post-merge to see if maintainability is improved.

There's a 2nd option we can keep in our back pocket of sharing type names between the two repos that would give the same level of abstraction without the added inefficiency.

Tratcher · 2020-01-17T22:05:42Z

src/libraries/Common/src/System/Net/Http/Http3/Frames/Http3ErrorCode.cs

+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+#if KESTREL
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3


IHttpHeadersHandler is an internal type in System.Net.Http.dll, it doesn't matter what the namespace is there.

IHttpHeadersHandler is a public (pubternal) type in Kestrel. Yes we can make API changes to pubternal types, that's not the question here. We should not use the System.Net.Http namespace for public types in Kestrel.

Tratcher · 2020-01-21T18:04:52Z

src/libraries/Common/src/System/Net/Http/Http3/QPack/QPackDecodingException.cs

+using System.Runtime.Serialization;
+
+#if KESTREL
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3.QPack


Reminder, please get rid of the Kestrel namespace for all internal types.

carlossanlop · 2020-01-21T20:52:38Z

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

@@ -203,6 +203,7 @@ public static partial class HttpVersion
        public static readonly System.Version Version10;
        public static readonly System.Version Version11;
        public static readonly System.Version Version20;
+        public static readonly System.Version Version30;


@scalablecory I noticed you're adding new APIs in the ref files. Please make sure to have all these new public API documented in the implementation (src files) with triple slash comments so that we can automatically port them in the future.

Tratcher · 2020-01-21T21:53:02Z

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

+
+            if (request.HasHeaders && request.Headers.Host != null)
+            {
+                BufferLiteralHeaderWithStaticNameReference(H3StaticTable.Authority, request.Headers.Host);


jkotalik · 2020-01-22T16:17:35Z

src/libraries/Common/src/System/Net/Http/Http2/IHttpHeadersHandler.cs

@@ -17,6 +17,8 @@ namespace System.Net.Http
 #endif
    interface IHttpHeadersHandler
    {
+        void OnStaticIndexedHeader(int index);


Did @Tratcher chat with you about the issues with these added interface definitions?

scalablecory · 2020-01-27T06:03:10Z

/azp list

azure-pipelines · 2020-01-27T06:03:14Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr crossgen2 runtime-libraries outerloop runtime-libraries outerloop-windows runtime-libraries outerloop-linux runtime-libraries outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-live-build runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono

scalablecory · 2020-01-27T06:03:40Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-01-27T06:03:54Z

Azure Pipelines successfully started running 1 pipeline(s).

scalablecory · 2020-01-27T07:50:43Z

CI failures appear to be an unrelated mix of flakey CI and #2131

scalablecory added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net.Http labels Jan 4, 2020

scalablecory requested review from a team January 4, 2020 13:26

scalablecory self-assigned this Jan 4, 2020

jkotalik reviewed Jan 5, 2020

View reviewed changes

gfoidl reviewed Jan 5, 2020

View reviewed changes

jkotalik reviewed Jan 6, 2020

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs Outdated Show resolved Hide resolved

jkotalik reviewed Jan 6, 2020

View reviewed changes

Tratcher reviewed Jan 6, 2020

View reviewed changes

GrabYourPitchforks reviewed Jan 6, 2020

View reviewed changes

src/libraries/Common/src/System/Net/Http/Http3/Helpers/VariableLengthIntegerHelper.cs Show resolved Hide resolved

src/libraries/Common/src/System/Net/Http/Http3/QPack/QPackEncoder.cs Outdated Show resolved Hide resolved

Tratcher reviewed Jan 6, 2020

View reviewed changes

stephentoub reviewed Jan 17, 2020

View reviewed changes

jkotalik reviewed Jan 17, 2020

View reviewed changes

Tratcher reviewed Jan 17, 2020

View reviewed changes

scalablecory force-pushed the http3 branch from ee5579c to 2ccf9b2 Compare January 19, 2020 07:31

Tratcher reviewed Jan 21, 2020

View reviewed changes

carlossanlop reviewed Jan 21, 2020

View reviewed changes

carlossanlop added the new-api-needs-documentation label Jan 21, 2020

Tratcher reviewed Jan 21, 2020

View reviewed changes

scalablecory force-pushed the http3 branch from 4d0c8b4 to 5dc22af Compare January 22, 2020 09:26

jkotalik reviewed Jan 22, 2020

View reviewed changes

jkotalik approved these changes Jan 22, 2020

View reviewed changes

Add HTTP/3 draft 25 support.

ba6c867

scalablecory force-pushed the http3 branch from f23f5a1 to ba6c867 Compare January 27, 2020 06:01

scalablecory merged commit 6f6c94d into dotnet:master Jan 27, 2020

This was referenced Feb 19, 2020

HttpClient operations now fail if ExecutionContext flow is suppressed #32524

Closed

HttpClient now leaks NetworkChange.NetworkAddressChanged handlers #32526

Closed

davidsh removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 19, 2020

davidsh added this to the 5.0 milestone Feb 19, 2020

scalablecory deleted the http3 branch July 29, 2020 16:05

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

		@@ -0,0 +1,105 @@
		// Copyright (c) .NET Foundation. All rights reserved.

	public static bool TryReadIntegerPair(ReadOnlySpan<byte> buffer, out long a, out long b, out int bytesRead)
	public static bool TryReadVariableLengthIntegerPair(ReadOnlySpan<byte> buffer, out long a, out long b, out int bytesRead)

	throw new QPackDecodingException("TODO sync with corefx" /CoreStrings.QPackHuffmanError, /, ex);
	throw new QPackDecodingException("TODO sync with runtime/libraries" /CoreStrings.QPackHuffmanError, /, ex);

	byte[] dataFrame = _dataFrame;
	byte[] dataFrame = _dataFrame ??= new byte[9];

+                      {
+                      }
+                      // TODO

	maxAge = Math.Min(maxAge.GetValueOrDefault(), maxAgeTmp);
	maxAge = Math.Min(maxAge.Value, maxAgeTmp);


		int lowerCase = ch \| 0x20;

		if ((uint)(lowerCase - 'a') <= 'f' - 'a') // lowerCase >= 'a' && lowerCase <= 'f'

Add HTTP/3 #1294

Add HTTP/3 #1294

Conversation

scalablecory commented Jan 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scalablecory Jan 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scalablecory Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scalablecory commented Jan 4, 2020 •

edited

Loading

scalablecory Jan 5, 2020 •

edited

Loading

stephentoub Jan 17, 2020 •

edited

Loading

scalablecory Jan 6, 2020 •

edited

Loading

Tratcher Jan 6, 2020 •

edited

Loading