[API Proposal]: Add property bag to QuicConnection

[JamesNK]

Background and motivation

QuicListenerOptions.ConnectionOptionsCallback is a callback that allows the server to modify connection options when it accepts a connection. One of its arguments is QuicConnection.

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListenerOptions.cs

Lines 33 to 36 in a54a823

	/// <summary>
	/// Selection callback to choose inbound connection options dynamically.
	/// </summary>
	public Func<QuicConnection, SslClientHelloInfo, CancellationToken, ValueTask<QuicServerConnectionOptions>> ConnectionOptionsCallback { get; set; } = null!;

In ASP.NET Core we allow people to hook into this callback, although we wrap it in our own signature because we have an abstraction called ConnectionContext for a server connection that's not specific to QUIC or HTTP/3.

_quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (quicConnection, helloInfo, cancellationToken) =>
    {
        var serverAuthenticationOptions = await _tlsConnectionOptions.OnConnection(new TlsConnectionCallbackContext
        {
            CancellationToken = cancellationToken,
            ClientHelloInfo = helloInfo,
            State = _tlsConnectionOptions.OnConnectionState,
            Connection = new QuicConnectionContext(quicConnection, _context), // wrap QuicConnection with our abstraction here
        });
        var connectionOptions = new QuicServerConnectionOptions
        {
            ServerAuthenticationOptions = serverAuthenticationOptions,
            IdleTimeout = options.IdleTimeout,
            MaxInboundBidirectionalStreams = options.MaxBidirectionalStreamCount,
            MaxInboundUnidirectionalStreams = options.MaxUnidirectionalStreamCount,
            DefaultCloseErrorCode = 0,
            DefaultStreamErrorCode = 0,
        };
        return connectionOptions;
    }
}

Once the connection is accepted and returned from

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = new QuicConnectionContext(quicConnection, _context);

The code above is creating our abstraction - QuicConnectionContext - twice: once in the callback and then again after AcceptConnectionAsync returns. We don't want to do this because of allocations, but also because someone can update state on our connection abstraction, and that would disappear if it was recreated.

It would be better if we were able to create QuicConnectionContext once inside the callback, use it, then add it to a property bag on QuicConnection, and then use that value when AcceptConnectionAsync returns.

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = (QuicConnectionContext)quicConnection.Properties[QuicConnectionContextKey];

Other options include adding QuicConnectionContext to a ConditionalWeakTable<QuicConnection, QuicConnectionContext> inside the callback, then removing it outside the callback. But a place to stash state on the QuicConnection would be much simpler.

This isn't critical for .NET 7. Workarounds include using a weak table, allocating twice (which fixes one problem but creates another...), or leaving ConnectionContext null in the callback in .NET 7 and waiting for this API in .NET 8.

API Proposal

namespace System.Net.Quic;

public class QuicConnection
{
    public IDictionary<string, object?> Properties { get; }
}

API Usage

_quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (quicConnection, helloInfo, cancellationToken) =>
    {
        quicConnection.Properties[QuicConnectionContextKey] = new QuicConnectionContext(quicConnection, _context);
        
        // ...figure out conneciton options...

        return connectionOptions;
    }
}

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = (QuicConnectionContext)quicConnection.Properties[QuicConnectionContextKey];

Alternative Designs

Add a state argument to ConnectionOptionsCallback callback and AcceptConnectionAsync.

_quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (quicConnection, helloInfo, state, cancellationToken) =>
    {
        var listenerContext = (QuicListenerContext)state!
        listenerContext._acceptingQuicConnectionContext = new QuicConnectionContext(quicConnection, _context);
        
        // ...figure out conneciton options...

        return connectionOptions;
    }
}

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken, state: this);
var connectionContext = _acceptingQuicConnectionContext; // was set in callback

Risks

No response

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

QuicListenerOptions.ConnectionOptionsCallback is a callback that allows the server to modify connection options when it accepts a connection. One of its arguments is QuicConnection.

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListenerOptions.cs

Lines 33 to 36 in a54a823

	/// <summary>
	/// Selection callback to choose inbound connection options dynamically.
	/// </summary>
	public Func<QuicConnection, SslClientHelloInfo, CancellationToken, ValueTask<QuicServerConnectionOptions>> ConnectionOptionsCallback { get; set; } = null!;

In ASP.NET Core we allow people to hook into this callback, although we wrap it in our own signature because we have an abstraction called ConnectionContext for a server connection that's not specific to QUIC or HTTP/3.

_quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (quicConnection, helloInfo, cancellationToken) =>
    {
        var serverAuthenticationOptions = await _tlsConnectionOptions.OnConnection(new TlsConnectionCallbackContext
        {
            CancellationToken = cancellationToken,
            ClientHelloInfo = helloInfo,
            State = _tlsConnectionOptions.OnConnectionState,
            Connection = new QuicConnectionContext(quicConnection, _context), // wrap QuicConnection with our abstraction here
        });
        var connectionOptions = new QuicServerConnectionOptions
        {
            ServerAuthenticationOptions = serverAuthenticationOptions,
            IdleTimeout = options.IdleTimeout,
            MaxInboundBidirectionalStreams = options.MaxBidirectionalStreamCount,
            MaxInboundUnidirectionalStreams = options.MaxUnidirectionalStreamCount,
            DefaultCloseErrorCode = 0,
            DefaultStreamErrorCode = 0,
        };
        return connectionOptions;
    }
}

Once the connection is accepted and returned from

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = new QuicConnectionContext(quicConnection, _context);

The code above is creating our abstraction - QuicConnectionContext - twice: once in the callback and then again after AcceptConnectionAsync returns. We don't want to do this because of allocations, but also because someone can update state on our connection abstraction, and that would disappear if it was recreated.

It would be better if we were able to create QuicConnectionContext once inside the callback, use it, then add it to a property bag on QuicConnection, and then use that value when AcceptConnectionAsync returns.

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = (QuicConnectionContext)quicConnection.Properties[QuicConnectionContextKey];

Other options include adding QuicConnectionContext to a ConditionalWeakTable<QuicConnection, QuicConnectionContext> inside the callback, then removing it outside the callback. But a place to stash state on the QuicConnection would be much simpler.

This isn't critical for .NET 7. Workarounds include using a weak table, allocating twice (yuck), or leaving ConnectionContext null in the callback and waiting for this API in .NET 8.

API Proposal

namespace System.Net.Quic;

public class QuicConnection
{
    public IDictionary<string, object?> Properties { get; }
}

API Usage

_quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (quicConnection, helloInfo, cancellationToken) =>
    {
        quicConnection.Properties[QuicConnectionContextKey] = new QuicConnectionContext(quicConnection, _context);
        
        // ...figure out conneciton options...

        return connectionOptions;
    }
}

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = (QuicConnectionContext)quicConnection.Properties[QuicConnectionContextKey];

Alternative Designs

Add a state argument to ConnectionOptionsCallback callback and AcceptConnectionAsync.

_quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (quicConnection, helloInfo, state, cancellationToken) =>
    {
        var listenerContext = (QuicListenerContext)state!
        listenerContext._acceptingQuicConnectionContext = new QuicConnectionContext(quicConnection, _context);
        
        // ...figure out conneciton options...

        return connectionOptions;
    }
}

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken, state: this);
var connectionContext = _acceptingQuicConnectionContext; // was set in callback

Risks

No response

Author:	JamesNK
Assignees:	-
Labels:	api-suggestion, area-System.Net.Quic
Milestone:	-

[JamesNK]

Following on from the alternative design idea:

1. Is ConnectionOptionsCallback only invoked when AcceptConnectionAsync is called? And it is always finished before AcceptConnectionAsync is done. If it isn't then this approach won't work.
2. Passing in state isn't really necessary. We can set a _acceptingQuicConnectionContext field via closure.

[JamesNK]

I updated ASP.NET Core to create our abstraction inside the callback and get/set it on a field. It appears to work: dotnet/aspnetcore#42774

It feels a little hacky. A property bag on QuicConnection would be easier to follow.

[karelz]

Triage: We do not like the Property bag -- feels kind of very general-purpose, which bit us in the past on other APIs.
The alternative proposal sounds reasonable.

We expected that it is sufficient for 8.0 as we are past Platform Complete. Is it correct @JamesNK?

[JamesNK]

The alternative proposal doesn't work. The callback isn't run when AcceptConnectionAsync is called. It happens in a background thread. Saving state of the current connection via closure doesn't work. Neither will passing in state via AcceptConnectionAsync

That brings us back to the idea of adding a property bag to QuicConnection.

I will see whether using a ConditionalWeakTable works but it feels hacky compared to a collection on QuicConnection itself.

[ManickaP]

The alternative proposal doesn't work. The callback isn't run when AcceptConnectionAsync is called. It happens in a background thread.

And we can change that if we decide to support the state object. It's similar to my alternative design for QuicListener in #67560.

[ManickaP]

Triage: the decision must be done 8.0 (no matter which one) as this might alter an existing API. For that reason, this is a high prio.

[ManickaP]

Would simple object? StateObject property instead of dictionary work for you @JamesNK?

[JamesNK]

I think so. But a dictionary seems more flexible (multiple APIs could use it to stash information without overwriting each others changes) and it is like SocketsHttpHandler.Properties and HttpRequestMessage.Properties.

Are you worried about performance? The dictionary could be lazily allocated if it is used to make it pay for play.

[ManickaP]

So we had another discussion about this internally. We're not fans of the dictionary, despite being a similar approach as in HTTP stack (we obsoleted that API some time ago and replaced it with the Options class which makes us wary of this approach). We'd still prefer to use a single object instead.
Since ASP is the only consumer for this and you have a workaround, we decided to postpone the API for the time being and not introduce anything yet.
We can make the final decision when we have more people wanting this (or something similar) and we have more feedback/input on this.