Initial port of MsQuic transport #15375

jkotalik · 2019-10-24T16:18:52Z

This ports the Microsoft.AspNetCore.Server.Kestrel.Transports.MsQuic package to github. Follow up PRs will be done for our initial HTTP/3 support.

TODO (in this PR):

Add sample which only uses MsQuic (May require client work)
Flesh out README for Windows and Linux builds.

TODO (As follow up, will file separate issues for these):

Add logging throughout transport
Handle shut down correctly
Figure out client-side bedrock for functional testing
Add functional tests for MsQuic
Performance and Stress

I'm not how people want to go about reviewing/merging this code. Much of this will eventually be deleted once a corefx Quic story is figured out.

jkotalik · 2019-10-24T16:23:24Z

cc @nibanks

nibanks · 2019-10-24T16:33:44Z

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicEnums.cs

+        SUCCESS = 0,
+        PENDING = 459749,
+        CONTINUE = 0x800704DE,
+        OUT_OF_MEMORY = 0x8007000E,
+        INVALID_PARAMETER = 0x80070057,
+        INVALID_STATE = 0x8007139F,
+        NOT_SUPPORTED = 0x80004002,
+        NOT_FOUND = 0x80070490,
+        BUFFER_TOO_SMALL = 0x8007007A,
+        HANDSHAKE_FAILURE = 0x80410000,
+        ABORTED = 0x80004004,
+        ADDRESS_IN_USE = 0x80072740,
+        CONNECTION_TIMEOUT = 0x800704CF,
+        CONNECTION_IDLE = 0x800704D4,
+        INTERNAL_ERROR = 0x80004005,
+        SERVER_BUSY = 0x800704C9,
+        PROTOCOL_ERROR = 0x800704CD,
+        VER_NEG_ERROR = 0x80410001


These values are platform dependent.

Interesting. In some places where this happens we build native shims (one per platform with a consistent public API), in others we unify them in managed code. If we can know for sure that the Linux values are consistent, and the Windows values are consistent, then I'd suggest a managed mapping. Native shims mean different builds for each platform which is a lot of goop for little value :)

I'll find the linux values now and add them in. It may be a good idea to run this on linux first before merging.

I'm not sure if it was clear, but these error codes are not an exhaustive list. The platform layer can indicate platform specific error codes as well up.

On windows, I assume we can just do a check for a failing HRESULT.

The list above was based on the defined macros. Would you recommend not having this enum list at all and just checking for HRESULTs on Windows and the return value being <= 0 on Unix? It may allow us to give a more detailed error message.

No, I'm not saying that you shouldn't have this list. You should. But I just don't want you to assume that the list is exhaustive. A comment might be worth adding to the enum definition. As for how to determine if the QUIC_STATUS is a success or failure, just look at each platform's QUIC_FAILED macros.

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicEnums.cs

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicNativeMethods.cs

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicRegistration.cs

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicSession.cs

nibanks · 2019-10-24T16:51:09Z

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicSession.cs

+            return listener;
+        }
+
+        public ValueTask<QuicConnection> ConnectionOpenAsync(


This function isn't actually async.

Yeah this should be async. It should wait for the connected event to occur. I haven't invested a ton into the client side.

nibanks · 2019-10-24T16:53:27Z

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicStream.cs

+            return sendContext.TcsTask; // TODO make QuicStream implement IValueTaskSource?
+        }
+
+        public Task StartAsync(QUIC_STREAM_START flags)


Start is only async if you pass the async flag.

Which flag are you referring to?

QUIC_STREAM_START_FLAG_ASYNC

Oh yeah, that was a new one. I'll do the needful.

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicStream.cs

nibanks · 2019-10-24T17:02:19Z

src/Servers/Kestrel/Transport.MsQuic/Internal/WinSockNativeMethods.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Transport.MsQuic.Internal
+{
+    internal class WinSockNativeMethods


Socket addresses are platform specific.

nibanks · 2019-10-24T17:04:00Z

src/Servers/Kestrel/Transport.MsQuic/MsQuicTransportOptions.cs

+        public ushort MaxBidirectionalStreamCount { get; set; } = 100;
+        public ushort MaxUnidirectionalStreamCount { get; set; } = 10;
+        public string Alpn { get; set; } = "h3-23";
+        public string RegistrationName { get; set; } = "Quic";


Don't set non-zero defaults. Require the app to set an ALPN. And for the registration name at least default to something like asp.net so we can differentiate different code in the same process.

This was done due to a layering issue with Kestrel we need to figure out. Ideally, if Kestrel is configured to use HTTP/3, it itself will set the alpn and registration name. I'll remove them though and put them in the app as you are correct.

nibanks · 2019-10-24T17:04:45Z

src/Servers/Kestrel/Transport.MsQuic/MsQuicTransportOptions.cs

+        // TODO figure out how to make this fluent with new bedrock infrastructure
+        public X509Certificate2 Certificate { get; set; }
+
+        public TimeSpan IdleTimeout { get; set; } = Debugger.IsAttached ? TimeSpan.FromHours(1) : TimeSpan.FromSeconds(30); // TODO think about these limits.


The app needs to make these decisions. Not the API. You need to support an "unset" value to allow the transport default to apply.

nibanks · 2019-10-24T17:13:05Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicStream.cs

+            var inputOptions = new PipeOptions(MemoryPool, PipeScheduler.ThreadPool, PipeScheduler.Inline, 1024 * 1024, 1024 * 1024 / 2, useSynchronizationContext: false);
+            var outputOptions = new PipeOptions(MemoryPool, PipeScheduler.Inline, PipeScheduler.ThreadPool, 1024 * 64, 1024 * 64 / 2, useSynchronizationContext: false);


Don't create both pipes if unidirectional.

nibanks · 2019-10-24T17:33:41Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicStream.cs

+            _processingTask = ProcessSends();
+        }
+
+        private async Task ProcessSends()


I don't understand the point of ProcessSends.

analogrelay · 2019-10-24T17:32:58Z

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicEnums.cs

+        SUCCESS = 0,
+        PENDING = 459749,
+        CONTINUE = 0x800704DE,
+        OUT_OF_MEMORY = 0x8007000E,
+        INVALID_PARAMETER = 0x80070057,
+        INVALID_STATE = 0x8007139F,
+        NOT_SUPPORTED = 0x80004002,
+        NOT_FOUND = 0x80070490,
+        BUFFER_TOO_SMALL = 0x8007007A,
+        HANDSHAKE_FAILURE = 0x80410000,
+        ABORTED = 0x80004004,
+        ADDRESS_IN_USE = 0x80072740,
+        CONNECTION_TIMEOUT = 0x800704CF,
+        CONNECTION_IDLE = 0x800704D4,
+        INTERNAL_ERROR = 0x80004005,
+        SERVER_BUSY = 0x800704C9,
+        PROTOCOL_ERROR = 0x800704CD,
+        VER_NEG_ERROR = 0x80410001


Interesting. In some places where this happens we build native shims (one per platform with a consistent public API), in others we unify them in managed code. If we can know for sure that the Linux values are consistent, and the Windows values are consistent, then I'd suggest a managed mapping. Native shims mean different builds for each platform which is a lot of goop for little value :)

analogrelay · 2019-10-24T17:33:20Z

src/Servers/Kestrel/Transport.MsQuic/Internal/QuicEnums.cs

+    /// <summary>
+    /// Represents the status of an operation by MSQuic
+    /// </summary>
+    public enum QUIC_STATUS : uint


Let's internalize everything we can, even though this is prototype. Better to start from safety :)

nibanks · 2019-10-24T17:43:20Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicStream.cs

+                        status = HandleEventPeerSendClose();
+                    }
+                    break;
+                // TODO figure out difference between SEND_ABORT and RECEIVE_ABORT


Each pipe (send/recv) can be independently aborted.

nibanks · 2019-10-24T17:44:54Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicStream.cs

+            return QUIC_STATUS.SUCCESS;
+        }
+
+        protected QUIC_STATUS HandleEventRecv(QuicStream stream, ref NativeMethods.StreamEvent evt)


I don't quite understand the flush/task stuff in this function.

It's how we do backoff. When FlushAsync returns an incomplete Task, we need to tell msquic to stop sending data until that task is complete, that's why we return pending. When that task finishes (in AwaitFlush) we then resume the flow of data by calling EnableReceive.

Ok. Makes sense. Thanks for the explanation.

nibanks · 2019-10-24T17:46:50Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicConnectionListener.cs

+                        var connection = new QuicConnection(_registration, evt.Data.NewConnection.Connection, false);
+
+                        evt.Data.NewConnection.SecurityConfig = _secConfig.NativeObjPtr;
+                        var msQuicConnection = new MsQuicConnection(connection);
+                        _acceptConnectionQueue.Writer.TryWrite(msQuicConnection);


Once you return success from this (and not pending) then the connection is going to continue on, and you can start getting callbacks, even streams being open and callbacks on those. If you want to do this async accept design, then you should return pending, and only when accepted, do you SetParam(SecConfig) to completely accept the connection.

I'd mark this as a TODO as well. (TODO: Accept backpressure)

jkotalik · 2019-10-30T21:42:01Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicConstants.cs

+        private const uint ProtocolErrorLinux = 200000013;
+        private const uint VerNegErrorLinux = 200000014;
+
+        internal static Func<uint, string> ErrorTypeFromErrorCode = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? GetErrorFromWindows : (Func<uint, string>)GetErrorFromLinux;


This is a little bit clever s.t. we don't need to continuously do runtime checks.

Yep, I think that makes sense. We just need to do the platform check once rather than each time we formulate an error code.

jkotalik · 2019-10-30T21:43:20Z

src/Servers/Kestrel/Transport.MsQuic/Internal/SocketNativeMethods.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Transport.MsQuic.Internal
+{
+    internal class SocketNativeMethods


I'll add a linux version of this method in the next PR (when I can actually do testing on linux).

Actually, I'm just going to remove it as I think there should be a method in corefx that does this conversion for us.

nibanks · 2019-10-31T14:48:29Z

src/Servers/Kestrel/Transport.MsQuic/src/Internal/MsQuicEnums.cs

+    {
+        NONE = 0,
+        FAIL_BLOCKED = 0x1,
+        IMMEDIATE = 0x2


Can you make another pass through all these enums to make sure you aren't missing any. For instance, this one is missing QUIC_STREAM_START_FLAG_ASYNC = 0x0004.

Done, a few other things needed to be updated. Good call.

jkotalik · 2019-10-31T15:04:46Z

@davidfowl , @Tratcher , @halter73 can one of you do a quick passthrough of this PR to make sure nothing is atrociously wrong? We can make some TODOs if we find anything else to do.

src/Servers/Kestrel/Transport.MsQuic/README.md

src/Servers/Kestrel/Transport.MsQuic/src/Internal/MsQuicConstants.cs

src/Servers/Kestrel/Transport.MsQuic/src/Internal/UIntExtensions.cs

analogrelay · 2019-10-30T21:55:24Z

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicConstants.cs

+        private const uint ProtocolErrorLinux = 200000013;
+        private const uint VerNegErrorLinux = 200000014;
+
+        internal static Func<uint, string> ErrorTypeFromErrorCode = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? GetErrorFromWindows : (Func<uint, string>)GetErrorFromLinux;


Yep, I think that makes sense. We just need to do the platform check once rather than each time we formulate an error code.

src/Servers/Kestrel/Transport.MsQuic/Internal/MsQuicConstants.cs

src/Servers/Kestrel/Transport.MsQuic/README.md

src/Servers/Kestrel/Transport.MsQuic/src/Internal/MsQuicConstants.cs

src/Servers/Kestrel/Transport.MsQuic/src/Internal/MsQuicStatusHelper.cs

src/Servers/Kestrel/Transport.MsQuic/src/Internal/MsQuicConstants.cs

src/Servers/Kestrel/Transport.MsQuic/src/Internal/MsQuicStatusHelper.cs

…Helper.cs Co-Authored-By: Chris Ross <[email protected]>

…nts.cs Co-Authored-By: Stephen Halter <[email protected]>

jkotalik requested review from halter73, davidfowl, Tratcher and geoffkizer October 24, 2019 16:18

jkotalik requested a review from analogrelay as a code owner October 24, 2019 16:18

jkotalik added the area-servers label Oct 24, 2019

jkotalik added this to the 5.0.0-preview1 milestone Oct 24, 2019

github-actions bot added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 24, 2019