dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.26k stars 4.73k forks source link

Connection Abstractions #1793

Closed scalablecory closed 2 years ago

scalablecory commented 4 years ago

System.Net.Connections

Connections is an abstraction for composable connection establishment. It aims to improve layering separation and provide a standard extensibility model for making network connections.

Connections targets client/server implementations, and their users with advanced needs to plug in custom functionality. The latter is a heavily requested feature for HttpClient, and the Kestrel team has a handful of examples of users taking advantage of this pattern.

Connections brings .NET into parity with “modern” transport models such as Go’s dialers and Netty's channels. ASP.NET has a similar set of interfaces (“Bedrock Transports”) that this would supersede.

Basic API usage

At its most basic usage, System.Net.Connections is an abstraction over Socket.Accept and Socket.Connect. For this Socket usage today:

// server
using var listener = new Socket(SocketType.Stream, ProtocolType.Tcp);
listener.Bind(new IPEndPoint(IPAddress.IPv6Loopback, 0));
listener.Listen();
using Socket connection = await listener.AcceptAsync();
using Stream connectionStream = new NetworkStream(connection);

// client
using var socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
await socket.ConnectAsync(new DnsEndPoint("contoso.com", 80));
using var stream = new NetworkStream(socket);

The equivalent with System.Net.Connections is:

// server
using IConnectionFactory factory = new SocketsConnectionFactory(SocketType.Stream, ProtocolType.Tcp);
using IConnectionListener listener = await factory.BindAsync(new IPEndPoint(IPAddress.IPv6Loopback, 0));
using IConnection connection = await listener.AcceptAsync();
using Stream stream = connection.Stream;

// client
using IConnectionFactory factory = new SocketsConnectionFactory(SocketType.Stream, ProtocolType.Tcp);
using IConnection connection = await factory.ConnectAsync(new DnsEndPoint("contoso.com", 80));
using Stream stream = connection.Stream;

Composability

Composability has been modeled after Stream. For instance:

Stream stream;
stream = new NetworkStream(new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp));
stream = new BufferedStream(stream);
stream = new GZipStream(stream, CompressionLevel.Optimal);

While Streams compose the raw byte stream, Connections compose the establishment of that stream:

// Create a connection factory.
IConnectionFactory factory;
factory = new SocketsConnectionFactory(SocketType.Stream, ProtocolType.Tcp);
factory = new SslConnectionFactory(factory);

// Establish a connection.
IConnection connection = await factory.ConnectAsync(endPoint, options);
Stream stream = connection.Stream;

Beyond things like TLS and sockets, library implementation can separate some connection establishment logic into clean layers, as seen in HttpClient here:

// Setup connection factory base. Either use the user's custom connection factory, or sockets.
_tcpConnectionFactory =
    settings._connectionFactory != null
    ? (IConnectionFactory)new EatDisposeConnectionFactory(settings._connectionFactory)
    : new SocketsConnectionFactory(SocketType.Stream, ProtocolType.Tcp);

// Middleware that selects which endpoint to connect to, routing through proxies.
_tcpConnectionFactory = new TransportSelectionMiddleware(_tcpConnectionFactory);

// Middleware to setup TLS. If the user's custom connection factory already sets up TLS, it's a no-op. Otherwise, it does it for them.
_tcpConnectionFactory = new HttpsConnectionMiddleware(_tcpConnectionFactory);

Extensibility

Components can expose a connection factory to the user as an extension model. As an example, a user might implement a bandwidth monitoring extension for HttpClient:

IConnectionFactory factory;
factory = SocketsHttpHandler.CreateConnectionFactory();
factory = new BandwidthMonitoringMiddleware(factory);

SocketsHttpHandler handler = new SocketsHttpHandler();
handler.SetConnectionFactory(factory);

Requests for HttpClient extensibility

As an example of how users would make use of this, we’ve seen many requests for extensibility in HttpClient:

Go example

Here we use a SOCKS proxy with Go's HTTP client, via a dialer:

auth := proxy.Auth{
    User:     "YOUR_PROXY_LOGIN",
    Password: "YOUR_PROXY_PASSWORD",
}

dialer, err := proxy.SOCKS5("tcp", "contoso.com:12345", &auth, proxy.Direct)
tr := &http.Transport{Dial: dialer.Dial}
myClient := &http.Client{
    Transport: tr,
}

Netty example

And use Netty's channel pipelines to do the same:

ChannelPipeline p = ch.pipeline();
p.addFirst(new Socks5ProxyHandler(new InetSocketAddress("contoso.com", 12345), "YOUR_PROXY_LOGIN", "YOUR_PROXY_PASSWORD"));
p.addLast(new HttpClientCodec());

Connection Properties

Streams have implementation-specific functionality and properties. For instance, Socket.Shutdown() and various properties on SslStream. This means that, even when composing them, the user must still keep track of multiple layers:

using Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
using Stream networkStream = new NetworkStream(socket);
using SslStream sslStream = new SslStream(networkStream);
using StreamWriter writer = new StreamWriter(sslStream) { AutoFlush = true };

Console.WriteLine($"Connected via TLS: {sslStream.CipherAlgorithm} {sslStream.CipherStrength}");
writer.Write("Hello World.");
socket.Shutdown(SocketShutdown.Send);

This makes it challenging to build extensibility into a library's design:

With connections, this is cleaned up by allowing each layer in a composed connection to expose, override, or hide features from the previous layer. Abstractions can be used to avoid exposing specific implementation.

Here, our TLS implementation exposes a property while passing through unknown types to previous layers.

bool IConnectionProperties.TryGet(Type propertyKey, [NotNullWhen(true)] out object? property)
{
    if ((propertyKey == typeof(ISslConnectionInformation))
    {
        property = (TProperty)(object)...;
        return true;
    }

    return _baseConnection.ConnectionProperties.TryGet(propertyKey, out property);
}

The type keys available in properties are not discoverable :( documentation must be read.

When using an established connection, we then extract it as well as a Socket property which was exposed by a previous sockets layer:

using IConnection connection = ...;
using StreamWriter writer = new StreamWriter(connection.Stream) { AutoFlush = true };

if (connection.ConnectionProperties.TryGet(out ISslConnectionProperties? sslProperties))
{
    Console.WriteLine($"Connected via TLS: {sslProperties.CipherAlgorithm} {sslProperties.CipherStrength}");
}

writer.Write("Hello World.");

if (connection.ConnectionProperties.TryGet(out Socket? socket))
{
    socket.Shutdown(SocketShutdown.Send);
}

Connection Establishment properties

Property bags are also used when establishing new connections, to allow each layer to decide how to establish the connection. For instance, HttpClient can implement a factory that tests if HTTPS is being used and inject TLS into a connection:

async ValueTask<IConnection> ConnectAsync(EndPoint endPoint, IConnectionProperties options, CancellationToken cancellationToken = default)
{
    if (!options.TryGet(out IHttpInternalConnectInfo httpProperties))
    {
        throw new HttpRequestException($"{nameof(HttpsConnectionMiddleware)} requires the {nameof(InternalConnectionInfoProperty)} connection property.");
    }

    HttpConnectionKind kind = httpProperties.Pool.Kind;

    if(kind == HttpConnectionKind.Https || kind == HttpConnectionKind.SslProxyTunnel)
    {
        return await _tlsConnectionFactory.ConnectAsync(endPoint, options, cancellationToken);
    }
    else
    {
        return await _plaintextConnectionFactory.ConnectAsync(endPoint, options, cancellationToken);
    }
}

Usage Examples

Establish a new connection and send data

async Task Send(IConnectionFactory factory)
{
    await using IConnection connection = await factory.ConnectAsync(new DnsEndPoint("contoso.com", 80));
    await using Stream s = connection.Stream;
    await using var sw = new StreamWriter(s);

    await sw.WriteAsync("GET / HTTP/1.1\r\n\r\n");
}

Listen for a new connection and receive data

async Task Receive(IConnectionListenerFactory factory)
{
    await using IConnectionListener listener = await factory.BindAsync(new IPEndPoint(IPAddress.Loopback, 0));
    await using IConnection connection = await listener.AcceptAsync();
    await using Stream s = connection.Stream;
    using var sr = new StreamReader(s);

    string requestLine = await sr.ReadLineAsync();
}

Proposed API

interface IConnectionProperties
{
    bool TryGet(Type propertyKey, [NotNullWhen(true)] out object? property);
}

// "easy mode" implementation for users; library implementors can write a custom one that merges allocation of IConnection, IConnectionProperties, and their properties.
public sealed partial class ConnectionPropertyCollection : IConnectionProperties
{
    public void Add<T>(T property);
    public bool TryGet(Type propertyKey, [NotNullWhen(true)] out object? property);
}

// this is separate from IConnection due to anticipation of QUIC. See QUIC straw man below.
interface IConnectionStream : IAsyncDisposable, IDisposable
{
    IConnectionProperties ConnectionProperties { get; }

    // If only one is implemented, the other should wrap. To prevent usage errors, whichever is retrieved first, the other one should throw.
    Stream Stream { get; }
    IDuplexPipe Pipe { get; }
}

interface IConnection : IConnectionStream
{
    EndPoint? LocalEndPoint { get; }
    EndPoint? RemoteEndPoint { get; }
}

// This could be split into two interfaces, one which has Connect and the other which has Bind. This would harm composability, but would avoid users needing to throw NotImplementedException if they only care about server-side.
interface IConnectionFactory : IAsyncDisposable, IDisposable
{
    ValueTask<IConnection> ConnectAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
    ValueTask<IConnectionListener> BindAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
}

interface IConnectionListener : IAsyncDisposable, IDisposable
{
    IConnectionProperties ListenerProperties { get; }
    EndPoint? LocalEndPoint { get; }
    ValueTask<IConnection> AcceptAsync(IConnectionProperties? options = null, CancellationToken cancellationToken = default);
}

static class ConnectionExtensions
{
    // Injects a simple stream filter without all the other ceremony.
    public static IConnectionFactory Filter(this IConnectionFactory factory, Func<IConnection, IConnectionProperties?, CancellationToken, ValueTask<Stream>> filter);
    public static IConnectionFactory Filter(this IConnectionFactory factory, Func<IConnection, IConnectionProperties?, CancellationToken, ValueTask<IConnection>> filter);

    // Generic wrapper for non-generic IConnectionProperties method.
    public static bool TryGet<T>(this IConnectionProperties properties, [MaybeNullWhen(false)] out T property);
}

Some thoughts:

Additional APIs

This integrates the above interfaces with HTTP, Sockets, TLS

namespace System.Net.Connections
{
    // needs documentation: exposes typeof(Socket) property in its connections.
    class SocketsConnectionFactory : IConnectionFactory
    {
        // dual-mode IPv6 socket. See Socket(SocketType socketType, ProtocolType protocolType)
        public SocketsConnectionFactory(SocketType socketType, ProtocolType protocolType);

        // See Socket(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)
        public SocketsConnectionFactory(AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType);

        public ValueTask<IConnectionListener> BindAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
        public ValueTask<IConnection> ConnectAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);

        public void Dispose();
        protected virtual void Dispose(bool disposing);
        public virtual ValueTask DisposeAsync();

        // These exist to provide an easy way for users to override default behavior.
        // A more idiomatic (but more API-heavy) way to do this would be to pass some sort of ISocketConfiguration that has all the pre-connect socket options one could want.
        protected virtual Socket CreateSocket(SocketType socketType, ProtocolType protocolType, EndPoint? endPoint, IConnectionProperties? options);
        protected virtual Stream CreateStream(Socket socket, IConnectionProperties? options);
        protected virtual IDuplexPipe CreatePipe(Socket socket, IConnectionProperties? options);
    }

    [System.CLSCompliantAttribute(false)] // due to TlsCipherSuite
    interface ISslConnectionProperties
    {
        CipherAlgorithmType CipherAlgorithm { get; }
        int CipherStrength { get; }
        HashAlgorithmType HashAlgorithm { get; }
        int HashStrength { get; }
        ExchangeAlgorithmType KeyExchangeAlgorithm { get; }
        int KeyExchangeStrength { get; }
        X509Certificate LocalCertificate { get; }
        SslApplicationProtocol NegotiatedApplicationProtocol { get; }
        TlsCipherSuite NegotiatedCipherSuite { get; }
        X509Certificate RemoteCertificate { get; }
        SslProtocols SslProtocol { get; }
        TransportContext TransportContext { get; }
    }

    // needs documentation: exposes typeof(ISslConnectionProperties) property in its connections.
    // needs documentation: requires SslClientAuthenticationOptions and SslServerAuthenticationOptions options in connect/bind.
    class SslConnectionFactory : IConnectionFactory
    {
        public SslConnectionFactory(IConnectionFactory baseFactory);

        public void Dispose();
        protected virtual void Dispose(bool disposing);
        public virtual ValueTask DisposeAsync();

        public ValueTask<IConnectionListener> BindAsync(EndPoint endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
        public ValueTask<IConnection> ConnectAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
    }
}

namespace System.Net.Http
{
    sealed class SocketsHttpHandler
    {
        public static IConnectionFactory CreateConnectionFactory();
        public static IConnectionFactory CreateConnectionFactory(Func<HttpRequestMessage, DnsEndPoint, IConnectionProperties, Socket> createSocket);

        // Return a stream that isn't Socket-based.
        public static IConnectionFactory CreateConnectionFactory(Func<HttpRequestMessage, DnsEndPoint, IConnectionProperties, CancellationToken, ValueTask<Stream>> establishConnection);
        public static IConnectionFactory CreateConnectionFactory(Func<HttpRequestMessage, DnsEndPoint, IConnectionProperties, CancellationToken, ValueTask<IConnection>> establishConnection);

        // For users of the above two APIs, they can call this if they just want to wrap the defaults.
        public static Socket DefaultCreateSocket(HttpRequestMessage message, DnsEndPoint endPoint, IConnectionProperties options);
        public static ValueTask<IConnection> DefaultEstablishConnection(HttpRequestMessage message, DnsEndPoint endPoint, IConnectionProperties options, CancellationToken cancellationToken);

        public void SetConnectionFactory(IConnectionFactory factory);

        // Sets a pre-encryption filter.
        public void SetConnectionFilter(Func<HttpRequestMessage, DnsEndPoint, IConnection, CancellationToken, ValueTask<Stream>> filter);
        public void SetConnectionFilter(Func<HttpRequestMessage, DnsEndPoint, IConnection, CancellationToken, ValueTask<IConnection>> filter);
    }
}

Thoughts and Questions

Future QUIC amendments

QUIC is not yet out of draft status, so QUIC-specific APIs were not a focus for this proposal. However, current knowledge of QUIC did help shape the APIs to make adapting it easier. Here are some thoughts based on current experience:

A QUIC extension to this might look like:


interface IMultiplexedConnection : IAsyncDisposable, IDisposable
{
    EndPoint? RemoteEndPoint { get; }
    EndPoint? LocalEndPoint { get; }

    ValueTask<IConnectionStream> OpenStream(IConnectionProperties? options = null, CancellationToken cancellationToken = default);
    ValueTask<IConnectionStream> AcceptStream(IConnectionProperties? options = null, CancellationToken cancellationToken = default);
}

interface IMultiplexedConnectionFactory : IAsyncDisposable, IDisposable
{
    ValueTask<IMultiplexedConnection> ConnectAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
    ValueTask<IMultiplexedConnectionListener> BindAsync(EndPoint? endPoint, IConnectionProperties? options = null, CancellationToken cancellationToken = default);
}

interface IMultiplexedConnectionListener : IAsyncDisposable, IDisposable
{
    IConnectionProperties ListenerProperties { get; }
    EndPoint? LocalEndPoint { get; }
    ValueTask<IMultiplexedConnection> AcceptAsync(IConnectionProperties? options = null, CancellationToken cancellationToken = default);
}

Alternately, the IConnection and IMultiplexedConnection APIs might be merged, reducing surface area significantly. The TCP version would simply throw if opening/accepting more than once. This API might look like:

interface IConnection
{
    EndPoint? LocalEndPoint { get; }
    EndPoint? RemoteEndPoint { get; }

    ValueTask<IConnectionStream> OpenStream(IConnectionProperties? options = null, CancellationToken cancellationToken = default);
    ValueTask<IConnectionStream> AcceptStream(IConnectionProperties? options = null, CancellationToken cancellationToken = default);
}

However, beyond API surface reduction there isn't clear practical reason to merge them. Given how QUIC is significantly different from TCP, it isn't clear that libraries would see correct reuse of filtering IConnectionFactory implementations.

JamesNK commented 4 years ago

Want: Pipelines support

scalablecory commented 4 years ago

Want: Pipelines support

It's there!

bgrainger commented 4 years ago

How many 3rd party libraries are in need of this and would use it if available?

MySqlConnector would probably use this. It currently has a homemade version of something similar: the base TCP socket or Unix domain socket connection can optionally have a SslStream-based connection layered on top of it (if the connection is secure), and either of those can have protocol compression layered on top of them. (I say "maybe" because the MySQL protocol has some quirks that make it not seamlessly composable.) There are also currently server load-balancing features that might be more cleanly expressed as another IConnectionFactory layer.

The connection factory could be exposed to advanced users for many of the same scenarios you've mentioned for HttpClient: bandwidth monitoring, protocol (SQL) inspection, in-memory (or alternate) transport, etc.

ReubenBond commented 4 years ago

How many 3rd party libraries are in need of this and would use it if available?

Not 3rd party, but Orleans uses the Bedrock abstractions and we will use this when it becomes available. We can help validate the APIs early, if that may be useful to you.

Having both Stream and Pipe exposed is a little confusing to me - how will that work?

scalablecory commented 4 years ago

I say "maybe" because the MySQL protocol has some quirks that make it not seamlessly composable.

@bgrainger Can you give more detail? The goal is to have a very flexible model for composability, and it would be great to know if there's a flaw we can address.

Having both Stream and Pipe exposed is a little confusing to me - how will that work?

@ReubenBond Stream and Pipelines are both in broad use, and this is a way for us to enable wide adoption of the abstraction.

The intended behavior is that if only one is implemented, the other should wrap that. To avoid usage errors, once one propery is used calling the other will throw an exception.

We can help validate the APIs early, if that may be useful to you.

Awesome! We have some of our own validation planned already; I will ping you with details once it's in that stage.

bgrainger commented 4 years ago

This is complicated to explain briefly, but I'll try. Each packet in the MySQL protocol has a monotonically increasing "sequence number" in its header. Using compression in the protocol takes an arbitrary number of uncompressed packets, compresses them all together, then writes them as the payload of one or more compressed packets, which have their own sequence numbers. You might expect that this compression would be transparent to the underlying data, but it's not. When the uncompressed packets require more than one compressed packet (because the compressed data exceeds the 2^24 byte size limit), then the sequence numbers of the uncompressed packets get reset, starting from the sequence number of the compressed packet that contains them.

For example, if we have uncompressed packets 1-80, and packets 1-50 become the (compressed) payload of compressed packet 1, and packets 51-80 become the (compressed) payload of compressed packet 2, then packets 51-80 get renumbered 2-31 before compression. So there has to be coordination between the compression stream and the layers around it.

halter73 commented 4 years ago

I started testing out the non-multiplexed connection abstractions in Kestrel's Socket transport, and here are a few things I noticed.

I haven't updated Kestrel's HttpsConnectionMiddleware to be a IConnectionListenerFactory decorator, but I could try that if people are interested.

I haven't tried out the multiplexed interfaces, but I think that having IMultiplexedConnection implement IConnectionStream will be problematic since neither the Pipe nor Stream properties will be usable.

scalablecory commented 4 years ago

Thanks for taking time to prove this out.

IConnectionProperties not having a setter is annoying.

I explicitly left this out to push a compositional model, but am interested in where it breaks down. Can you give some specific examples of how you'd use a setter?

IConnectionProperties.TryGet not having a generic version on the interface removes some opportunities for dead code elimination in custom implementations like Kestrel's TransportConnection.

FWIW we have loosely agreed to get rid of the type-based model all-together and instead have the property key just be object so there is no accidental shadowing. Available keys would be communicated via static properties similar to WPF dependency properties.

Can you give an example of where a generic version helps? We can revisit that decision if you've found it makes a significant difference.

  • IConnection not having an Abort() API means that Kestrel uses IConnectionLifetimeFeature.Abort() and stores the IConnectionLifetimeFeature in ConnectionProperties. How are we supposed to abort connections? Disposing the Stream? Our experience in ASP.NET is that people dispose Streams when they're done using it, not necessarily when they want to close the connection.

Beyond some Abort() method, there's also a consideration of how to communicate such an abort from the remote end to the local Stream or Pipe. How will kestrel react to e.g. a SocketException with one implementation vs a CustomProtocolResetException in another?

Right now, it's intentionally left undefined: our experience working with QUIC revealed a very TCP-centric mental model for aborting. Trying to make an abort API generic between just those two doesn't seem possible, let alone considering any other random protocol (does a UDS have compatible concepts of abort/reset/etc.?). Do you feel like a feature will work for now (at least, for this first pass in .NET 5), or do you think it's worth some brainstorming?

No CancellationToken for IConnectionListener.DisposeAsync/UnbindAsync could make it difficult to communicate to the listener when to switch from a graceful to an abortive shutdown, but right now Kestrel doesn't need this.

See above :)

No metadata on IConnectionListener is annoying. This makes it impossible for Kestrel to communicate what port it really bound to when asked to bind to port 0.

Agreed, it makes sense to add a property bag there.

halter73 commented 4 years ago

I explicitly left this out to push a compositional model, but am interested in where it breaks down. Can you give some specific examples of how you'd use a setter?

Given that filters already need to wrap the IConnection to replace the Stream or Pipe, this isn't actually so bad by comparison. I think the pain came from adapting existing Kestrel code that expects a more mutable IConnection/ConnectionContext.

Kestrel's HttpsConnectionMiddleware adds ITlsApplicationProtocolFeature to ConnectionProperties. To do this HttpsConnectionMiddleware needs to wrap IConnection, IConnection.Stream/Pipe and IConnection.ConnectionProperties with custom implementations. This is more painful than just setting the Stream/Pipe on a mutable IConnection and adding a property to a mutable IConnectionProperties.

This is definitely not a huge issue though. I think it's mostly a matter of preference. With the wrapping approach, you don't need lines like context.Transport = originalTransport in a finally when unwinding. HttpsConnectionMiddleware doesn't even unset ITlsApplicationProtocolFeature, but with the wrapping approach this happens implicitly which is nice.

Can you give an example of where a generic version helps? We can revisit that decision if you've found it makes a significant difference.

It makes a bigger difference the more features/properties there are and the further down the else if (typeof(TFeature) == typeof(IFeatureInQuestion)) chain we check for the feature/property. @benaadams ran some microbenchmarks a while back https://github.com/aspnet/KestrelHttpServer/pull/2290.

How will kestrel react to e.g. a SocketException with one implementation vs a CustomProtocolResetException in another?

Kestrel doesn't special-case exception types thrown from the transport except to log ConnectionResetExceptions (which are custom to Kestrel transports) at a lower level than most other IOExceptions.

Kestrel is probably somewhat unusual where it expects writing to the transport to never fail even if the connection has been closed by the client or aborted by the server app. The only way Kestrel can observe the connection failing is either by observing an exception when reading from the transport or a ConnectionClosed CancellationToken firing.

Kestrel will probably have to adapt any shared transport to have the unusual write-never-throws behavior, but I do think we should add a ConnectionClosed or StreamClosed CancellationToken to IConnectionStream instead of relying on something in ConnectionProperties.

Right now, it's intentionally left undefined: our experience working with QUIC revealed a very TCP-centric mental model for aborting. Trying to make an abort API generic between just those two doesn't seem possible, let alone considering any other random protocol (does a UDS have compatible concepts of abort/reset/etc.?). Do you feel like a feature will work for now (at least, for this first pass in .NET 5), or do you think it's worth some brainstorming?

Yeah. Abort() not taking an error code is problematic for QUIC. In Kestrel we added a new Abort() that takes an error code and default to 0x102 (H3_INTERNAL_ERROR) for the parameterless version. I agree it's not great. I'm OK with implementing Abort() via ConnectionProperties, but that doesn't feel great either.

scalablecory commented 4 years ago

It makes a bigger difference the more features/properties there are and the further down the else if (typeof(TFeature) == typeof(IFeatureInQuestion)) chain we check for the feature/property. @benaadams ran some microbenchmarks a while back aspnet/KestrelHttpServer#2290.

Ahh clever.

halter73 commented 4 years ago

I also took a look at rewriting Kestrel’s HttpsConnectionMiddleware to use the proposed abstraction.

This went well aside from one key difference from the proposed SslConnectionListenerFactory which is that HttpsConnectionMiddleware wraps the IConnectionListener instead of IConnectionListenerFactory. The reason for this is that Kestrel needs to be able to configure connection middleware on a per-remote-endpoint basis. We could theoretically have a singleton SslConnectionListenerFactory keep track of mappings between each remote endpoint and its configuration, but Kestrel already easily does this, so I see no reason to make connection middleware do this given each middleware would likely have to come up with its own hand-rolled solution.

When rewriting the HttpsConnectionMiddleware, I wound up liking the wrapping the IConnection more than setting properties on ConnectionContext like we did before. It’s a little more verbose (about 33% more), but it really encourages writing low-allocation code, and it eliminates a class of bug where middleware doesn’t properly reset a property when exiting. If the middleware was simpler, I might find wrapping the IConnection a bit more onerous.

One thing that was really annoying when writing both the transport and middleware was writing the logic to throw when accessing the Stream after the IDuplexPipe and vice versa. This logic needs to be written in the transport, and then rewritten at each layer that wraps the Stream/IDuplexPipe. Maybe we should write some built in type that you can construct with either a Stream or an IDuplexPipe, exposes both types as properties that throws if the other property was accessed first.

One thing I really like is this design allows for the possibility of writing a connection throttling middleware since it allows intercepting the call to IConnectionListener.AcceptAsync(). If we use this, we won’t need to add yet another extensibility point to Kestrel for this functionality as suggested in https://github.com/dotnet/aspnetcore/issues/13295.

scalablecory commented 4 years ago

API in issue updated with feedback from @stephentoub, @davidfowl, and @halter73

CC @JamesNK

Results from SmtpClient proving

The API worked great here, with one caveat:

Only supporting asynchronous APIs presents a significant hurdle for existing APIs to adopt without costly sync-over-async. Not supporting synchronous Dispose is particularly challenging, as there is a lot of established code doing a non-async using on e.g. Stream.

We should consider implementing synchronous APIs to make this story better.

davidfowl commented 4 years ago

Why do we need sync APIs and which ones exactly do you mean?

scalablecory commented 4 years ago

Why do we need sync APIs and which ones exactly do you mean?

So here's the driving concern: how do we get these interfaces into existing APIs that have sync support?

For instance, SmtpClient.Send has a full synchronous path right now. It uses sync Socket.Connect there. The moment it moves over to this new abstraction, it no longer has that ability. Similarly, we might also have HttpClient's new sync APIs use a sync connect.

We also see IDisposable being implemented for a lot of APIs right now, and so only supporting IAsyncDisposable means all those APIs have no great way to dispose of an IConnection.

It leads us with a few options:

I don't like any of these options, but we need to pick one. I'm leaning toward adding synchronous support being the least worst.

Drawaes commented 4 years ago

Then we never move forward because someone somewhere is pretending that async networking is actually sync.

benaadams commented 4 years ago

If needed, have a different set of interfaces IBlockingConnectionFactory, IBlockingConnection, etc don't conflate the different and conflicting modes of operation in one set of interfaces.

Also then support can be determined at compile time; rather than either throwing or hand waving at runtime.

Drawaes commented 4 years ago

That is a better idea because then someone writing a transport doesn't have to provide the sync methods.

halter73 commented 4 years ago

I think adding blocking networking APIs adds a lot of complexity for little benefit. As @Drawaes says, networking inherently async. I understand that the OS exposes blocking APIs, but I don't think that suddenly makes the blocking OK.

We should always be telling developers to do non-blocking I/O if they want to build a highly scalable apps. Blocking I/O still leads to threadpool starvation much faster than non-blocking I/O even without any sync-over-async. For developers who are porting low-traffic line-of-business apps that have always managed doing blocking I/O without threadpool starvation, I think sync-over-async is likely fine.

We could always go back and add IBlocking variants of the interfaces as @benaadams suggests if there's enough demand.

scalablecory commented 4 years ago

Lots of expert-focused opinions here. I think we can all agree async is what we'd prefer, but lets make sure we are looking at it from other perspectives as well. I don't view this as an expert-only API.

One of the goals of this is adoption into existing libraries. Lack of blocking APIs will hurt that goal.

We also have data (as outlined in HttpClient sync issue) that many users do not understand effective async, and that they still desire high performance sync APIs. Some are okay with horizontal rather than vertical scalability if it simplifies their code. There is value in enabling this scenario.

davidfowl commented 4 years ago

One of the goals of this is adoption into existing libraries. Lack of blocking APIs will hurt that goal.

Lets be specific though, lets talk about specific libraries and get some code samples. You might be right but lets get some concrete data about which libraries.

Can you outline the sync APIs you want to add?

JamesNK commented 4 years ago

cc @jtattermusch

scalablecory commented 4 years ago

Lets be specific though, lets talk about specific libraries and get some code samples.

SmtpClient and HttpClient.

Both need a sync Connect() mechanism, both keep a socket around that needs cleanup inside of their Dispose() methods.

We can implement IAsyncDisposable on both if we want, but this won't erase the large amount of existing user code that is doing a non-async using.

Can you outline the sync APIs you want to add?

Connect() and Dispose() for everything under IConnectionFactory. I see less use for blocking listener methods so I don't have strong opinions there, but consistency is worth considering.

benaadams commented 4 years ago

HttpClient only just had the sync api added; and it isn't highly discoverable. Also you shouldn't be newing up and disposing HttpClient per request in any high usage scenario as you will be leaving lots of sockets in a TIME_WAIT state for 240 seconds. So it would only be low throughput scenerios when threadpool exhaustion due to Connect sync-over-async would be less important?

Not sure about SmtpClient does it have an async api?

omariom commented 4 years ago

SmtpClient is obsolete

davidfowl commented 4 years ago

IConnectionProperties is no longer type-based, but rather uses a property system inspired by WPF dependency properties. The new system has improved discoverability, is strongly typed, and helps prevent unintentional shadowing that can happen when using types.

I don't quite understand the benefits here. Improved discovery ability how? Strongly typed how? Can you show a before and after example of how it improved each of these?

scalablecory commented 4 years ago

HttpClient only just had the sync api added; and it isn't highly discoverable.

The new sync APIs are very discoverable first-class APIs on HttpClient.

And Dispose() already exists, which now needs to call IConnection.DisposeAsync().

Also you shouldn't be newing up and disposing HttpClient per request in any high usage scenario as you will be leaving lots of sockets in a TIME_WAIT state for 240 seconds. So it would only be low throughput scenerios when threadpool exhaustion due to Connect sync-over-async would be less important?

Yes, it's certainly sub-optimal but I think it's not the end of the world for typical HttpClient usage.

Keeping in mind that I'm not just worried purely about efficiency, but about adoptability too. Any library using this will need to know how to do sync-over-async against ValueTask, and due to lack of understanding around this we currently prefer APIs to only ever expose a ValueTask if the only thing we expect users to do with it is to directly await it.

Not sure about SmtpClient does it have an async api?

Both sync and async, with more or less the same usage pattern as HttpClient.

scalablecory commented 4 years ago

IConnectionProperties is no longer type-based, but rather uses a property system inspired by WPF dependency properties. The new system has improved discoverability, is strongly typed, and helps prevent unintentional shadowing that can happen when using types.

I don't quite understand the benefits here. Improved discovery ability how? Strongly typed how? Can you show a before and after example of how it improved each of these?

Before:

// zero discoverability -- no way to know if this key exists without reading docs.
if(properties.TryGet(out ISslConnectionInformation? sslInfo))
{
   // ...
}

// above is just a helper method that translates to this...
if(properties.TryGet(typeof(ISslConnectionInformation), out object? sslInfoObject) && sslInfoObject is ISslConnectionInformation sslInfo)
{
   // ...
}

After:

class SslConnectionFactory : IConnectionFactory
{
    // a new pattern factories should follow: property keys are used to lookup properties and can be discovered easily just by reading ref source. inspired by WPF dependency properties.
    public static ConnectionPropertyKey<ISslConnectionInformation> ConnectionInformationProperty { get; }
}

// no more object, and you can't accidentally do e.g. "out int".
if(properties.TryGet(SslConnectionFactory.ConnectionInformationProperty, out ISslConnectionInformation? sslInfo))
{
   // ...
}
davidfowl commented 4 years ago

The new sync APIs are very discoverable first-class APIs on HttpClient.

Not they aren't. If they were we'd had more overloads (matching the async ones). We intentionally made it "more advanced" because it's not the ideal or recommended usage.

@scalablecory That example isn't convincing to me. How did you know to look up by a static instance property on a random type?

scalablecory commented 4 years ago

@scalablecory That example isn't convincing to me. How did you know to look up by a static instance property on a random type?

I think the common use case will be people knowing exactly which factories they're using, and this gives them a path to finding which properties they expose/expect.

For the extensibility scenario, e.g. a custom transport plugin, this pattern could be extended to have HttpClient expose its own properties to indicate what transports might choose to implement.

stephentoub commented 4 years ago

SmtpClient is obsolete

SmtpClient is not obsolete. It's a bug in the docs. https://github.com/dotnet/dotnet-api-docs/issues/2986

scalablecory commented 4 years ago

Aggregating current outstanding discussions from here and in-person meetings. If I'm misrepresenting anyone's opinions here please edit with corrections.

jtattermusch commented 4 years ago

cc @wicharypawel

shaggygi commented 4 years ago

Just curious, shouldn't the namespace be System.IO.Connections as it could interact with different types of transports like serial ports, buses (I2C, SPI, GPIO, etc.), Bluetooth, etc.?

scalablecory commented 4 years ago

Just curious, shouldn't the namespace be System.IO.Connections as it could interact with different types of transports like serial ports, buses (I2C, SPI, GPIO, etc.), Bluetooth, etc.?

You make a good point. We shouldn't consider namespace final; some things will change here anyway,

scalablecory commented 4 years ago

API Updated @JamesNK @halter73

Changes:

More notes:

shaggygi commented 4 years ago

@scalablecory are there any preview packages we can use to start playing around with? Of course, with the understanding this is early and changes are expected. Any help is appreciated.

scalablecory commented 4 years ago

@scalablecory are there any preview packages we can use to start playing around with? Of course, with the understanding this is early and changes are expected. Any help is appreciated.

We're aiming to get this into a preview soon, once API review has been done.

shaggygi commented 4 years ago

@scalablecory Had troubles finding the review today. Could you add the link once posted on YouTube? Thx

halter73 commented 4 years ago

The API review was rescheduled for Thursday.

KrzysztofCwalina commented 4 years ago

In .NET, we try to avoid: a) interfaces, b) factories. Could this work as follows?

// server
using var socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
await socket.BindAsync(new IPEndPoint(IPAddress.IPv6Loopback, 0), cancellationToken);
using SocketConnection connection = socket.AcceptConnectionAsync(cancellationToken);
using Stream stream = connection.Stream;

// client
using var socket = new Socket(SocketType.Stream, ProtocolType.Tcp);
using SocketConnection = await socket.OpenConnectionAsync(new DnsEndPoint("contoso.com", 80), cancellationToken);
using Stream stream = connection.Stream;
omariom commented 4 years ago

In .NET, we try to avoid: a) interfaces

Have default interfaces methods changed the guidelines? Interfaces are now almost the same as abstract classes, except that classes can only be derived from once.

bartonjs commented 4 years ago

API review notes:

One potential alternative for the property lookup (in no way agreed upon, but took elements from the discussion):

public abstract class Connection
{
    public bool TryGetProperty<T>(out T value) where T : IConnectionPropertySet
    {
        object valueObject;

        if (!TryGetProperty(typeof(T), out valueObject))
        {
            return _parent != null && _parent.TryGetProperty(typeof(T), out valueObject);
        }

        value = (T)valueObject;
        return true;
    }

    public T GetProperty<T>() where T : IConnectionPropertySet
    {
        if (!TryGetProperty<T>(out T value))
        {
            throw Something;
        }

        return value;
    }

    protected virtual bool TryGetProperty(Type key, out T value);
}
benaadams commented 4 years ago

unresolved discussion that most, if not all, of these interfaces should be abstract classes.

If you have both server and client in an abstract class; then you don't have compile time help when only one is implemented. If they are in separate classes then you can't implement both. Also if they are classes then you can't inherit from SafeHandle; so would need a secondary indirection for everything if you did use it.

If there is an interface for server and client; then you get explicit compile time help if you only implement one; you can implement SafeHandle directly etc.

benaadams commented 4 years ago

<T> form of GetProperty is better than an Type, object as you'd (should) want to cast straight away; so the loose typing isn't great.

bartonjs commented 4 years ago

If they are in separate classes then you can't implement both.

Depending on what extension methods and overloads and the like existed, that'd actually be a goal. For interface-based approaches, if there were ambiguous-target methods, it would probably be good to write an analyzer that says to not implement both of them... just to avoid the problems later.

wicharypawel commented 4 years ago

@scalablecory, sorry for question out of the blue. Would it be possible to monitor connectivity state of underlying connections? Assuming that client keeps multiple of them, I would like to know their state, possibly using reactive approach based on observer pattern.

scalablecory commented 4 years ago

@wicharypawel a connection could implement some sort of IConnectionMonitor property, but the in-box ones will currently not -- the general guidance will be to create Nuget packages that can insert themselves into the layering to expose such features.

wicharypawel commented 4 years ago

@scalablecory thank you for your answer, as long as this would be possible to implement, it's fine to me.

shaggygi commented 4 years ago

I'm still a little confused on how the TryGetProperty will work. What if you have multiple properties of the same Type, how would it know when to get a specific property?

This question is similar to @stephentoub concerns in 1st review.

For example, if I have 2 properties of type int (i.e. int baudRate and int dataBits). How would you get each property individually since they are both int?

shaggygi commented 4 years ago

Reviews found here:

https://www.youtube.com/watch?v=a9WxNvINTmU https://www.youtube.com/watch?v=C5W7f10eX3Q https://github.com/dotnet/apireviews/tree/master/2020/05-xx-connection-abstractions