Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection Abstractions #1793

Closed
scalablecory opened this issue Jan 16, 2020 · 73 comments
Closed

Connection Abstractions #1793

scalablecory opened this issue Jan 16, 2020 · 73 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Milestone

Comments

@scalablecory
Copy link
Contributor

scalablecory commented Jan 16, 2020

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:

  • One can no longer just ask a user for a Stream, but instead every piece of layer that they need to override.
  • Multiple callbacks may be needed to hide certain layers from the user, to allow them to implement only exactly what they need.

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:

  • A purer version of the API could move EndPoint parameters/properties into the IConnectionProperties.

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

  • Heavy use of property bags significantly harms discoverability: users need to read documentation to understand what features to expect from each layer.
    • This is not a common design pattern in corefx, but our ASP.NET users will be very familiar with DI and should have an easy enough time grasping it.
    • This puts additional burden on maintainers to ensure thorough documentation of required/exposed properties.
  • Shuffling features between layers with property bags may harm maintainability if authors need to pay attention to which layer introduces which property and how they all mold together.
    • To prove out this API, I implemented it feature-complete for HttpClient and did not observe this complexity. I believe there are not a large number of use cases one would want to introduce layers for, and so layering is unlikely to become complex enough for this to be a problem in most apps.
  • This API has a very large surface area. Are there enough benefits versus a simpler one (e.g. a Func<(string host, int port), Stream>)?
    • HttpClient has a strong need for a connection establishment abstraction. All but one need could be solved by the simpler one.
    • ... however, HTTP/3 and QUIC is significantly more involved and will need some more complex dialer abstraction anyway. Having some symmetry between the TCP and QUIC dialers may be beneficial.
    • This being the standard/recommended method to abstract connections across BCL, ASP.NET Core, and 3rd party libraries provides value over some simple library-specific callbacks.
  • How much impact will this have? How many 3rd party libraries are in need of this and would use it if available?
    • Not many people need this, but when they do it is really important to them. See HttpClient extensibility asks.
    • There are not many 3rd party libraries opening network connections.
  • Should we apply the extensibility model to other things? Does it make sense for e.g. SmtpClient, SqlConnection, and so on to make use of this?
    • For now, we have decided to be conservative and wait to see how this gets used before introducing those extensibility points.

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:

  • QUIC has features that an app is required to be aware of. It is not possible for an app to write a generic protocol that is seamlessly usable between both TCP and QUIC without at least some small shim that knows how to do the QUIC-specific stuff:
    • There is no "keep sending in background after socket/process closed" concept, so some form of flushing of send buffers is required.
    • Streams are aborted with status codes, and can abort read and write side of a stream separately.
    • Connections are always closed with an status code.
    • There are no predefined codes to indicate success/failure: they are all application-specific. It's not clear what an abortive QuicStream.Dispose should do, for instance.
    • Protocols make use of unidirectional/bidirectional stream differentiation, and how those map to stream IDs.
  • IConnectionStream and IConnection are split in the proposed API specifically to later support an IMultiplexedConnection that creates multiple IConnectionStream.
    • To avoid an IMultiplexedConnection, we might choose to merge the two APIs and have IConnection force users to explicitly open the one bidirectional IConnectionStream for the connection.

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.

@scalablecory scalablecory added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net labels Jan 16, 2020
@scalablecory scalablecory added this to the 5.0 milestone Jan 16, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 16, 2020
@karelz karelz removed the untriaged New issue has not been triaged by the area owner label Feb 20, 2020
@JamesNK
Copy link
Member

JamesNK commented Feb 21, 2020

Want: Pipelines support

@scalablecory
Copy link
Contributor Author

Want: Pipelines support

It's there!

@bgrainger
Copy link
Contributor

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
Copy link
Member

ReubenBond commented Feb 22, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

halter73 commented Mar 23, 2020

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

  • IConnectionProperties not having a setter is annoying.
  • IConnectionProperties.TryGet not having a generic version on the interface removes some opportunities for dead code elimination in custom implementations like Kestrel's TransportConnection.
  • No ConnectionId as a top level property on IConnection makes logging difficult. Right now, Kestrel just uses an IConnectionIdFeature interface in ConnectionProperties, but it's not great.
  • EDIT: IConnection not having an Abort() API means that Kestrel uses IConnectionLifetimeFeature.Abort() and stores the IConnectionLifetimeFeature in ConnectionProperties. When we dispose the Stream or complete the pipe, we still expect all the written data to be flushed. Abort() means kill the connection immediately even if everything isn't flushed. This can happen after the Stream is disposed or Pipe is completed. I feel this should be a top-level API.
    • Additionally, 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.
  • 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.
  • 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.

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
Copy link
Contributor Author

scalablecory commented Mar 23, 2020

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
Copy link
Member

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 aspnet/KestrelHttpServer#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
Copy link
Contributor Author

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
Copy link
Member

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 dotnet/aspnetcore#13295.

@scalablecory
Copy link
Contributor Author

scalablecory commented Mar 31, 2020

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

  • 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.
  • SocketsHttpHandler implementation is significantly changed; it now implements both "easy mode" callbacks and full IConnectionFactory support. For simplicity it no longer allows the user to override the TLS implementation, though this window is left cracked open to allow for a future API if desired.
  • Add a virtual CreateSocket to the sockets factories for users wanting to set socket options pre-connect.
  • Add ListenerProperties and LocalEndPoint to IConnectionListener.
  • Add IConnectionFactory.Filter extension methods for easily injecting a stream filter with a callback instead of implementing multiple interfaces.
  • Add ConnectionFactory property to SmtpClient.

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
Copy link
Member

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

@scalablecory
Copy link
Contributor Author

scalablecory commented Mar 31, 2020

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:

  • Add synchronous support to these interfaces. This complicates implementations, but makes just using it a lot more flexible.
  • Tell people that IConnectionFactory is just not compatible with sync code, and libraries using it as an extensibility point should throw if you use sync APIs combined with a factory. This kills the usefulness of the API: instead of replacing code, it will just add more.
  • Tell people to use sync-over-async.

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
Copy link
Contributor

Drawaes commented Mar 31, 2020

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

@benaadams
Copy link
Member

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
Copy link
Contributor

Drawaes commented Mar 31, 2020

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

@halter73
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

JamesNK commented Mar 31, 2020

cc @jtattermusch

@scalablecory
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

omariom commented Apr 1, 2020

SmtpClient is obsolete

@davidfowl
Copy link
Member

davidfowl commented Apr 1, 2020

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
Copy link
Contributor Author

scalablecory commented Jul 28, 2020

Closing this issue (the PR has been merged). Remaining work is tracked in #40044.

@Pro100AlexHell
Copy link

I have simple task - detect remote IP address to which http client is connected. I don't want to DNS resolve, but need exactly remote IP.
I didn't understand how to detect remote IP in net core 3.1.
Also I have no time for reading all of topics regarding breaking changes and new API.

I have found solution for .net - https://stackoverflow.com/questions/6655713/how-to-get-ip-address-of-the-server-that-httpwebrequest-connected-to but it doesn't work as it is for dot net, but not core.

Answer please, my simple question.
I can use WebRequest or HttpClient or anything you suggest.

@davidfowl davidfowl reopened this Oct 8, 2020
@davidfowl
Copy link
Member

Reopening this since we reverted it in 5.0

@stephentoub stephentoub modified the milestones: 5.0.0, 6.0.0 Oct 9, 2020
@KieranDevvs
Copy link

KieranDevvs commented Oct 24, 2020

Will this include some form of Socket frame decoding/encoding support out of the box like netty in Java? One of my gripes with the .NET Socket is that you have to write so much boiler plate to get communications setup.

@davidfowl
Copy link
Member

@KieranDevvs see https://github.com/davidfowl/BedrockFramework

@wsq003
Copy link

wsq003 commented Dec 7, 2020

How many ProtocolType are we supposed to support? I would prefer: TCP, UDP, HTTP, WebSocket, gRPC, Protobuff, etc.

In application point of view, they are all communication level.

@shaggygi
Copy link
Contributor

Understanding possible .NET 6 features/roadmap has not been released yet, anyone know when we might start seeing focus back on this API?

@stephentoub
Copy link
Member

I'm going to close this. If we decide to revisit it later, it can be re-opened, but at present it's not actionable. Thanks.

@karelz karelz modified the milestones: Future, 7.0.0 Apr 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Projects
None yet
Development

No branches or pull requests