Open JamesNK opened 2 years ago
Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.
Author: | JamesNK |
---|---|
Assignees: | - |
Labels: | `api-suggestion`, `area-System.Net.Quic` |
Milestone: | - |
Following on from the alternative design idea:
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._acceptingQuicConnectionContext
field via closure.I updated ASP.NET Core to create our abstraction inside the callback and get/set it on a field. It appears to work: https://github.com/dotnet/aspnetcore/pull/42774
It feels a little hacky. A property bag on QuicConnection would be easier to follow.
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?
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.
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 https://github.com/dotnet/runtime/issues/67560.
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.
Would simple object? StateObject
property instead of dictionary work for you @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.
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.
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 isQuicConnection
.https://github.com/dotnet/runtime/blob/a54a823ece1094dd05b7380614bd43566834a8f9/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListenerOptions.cs#L33-L36
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.Once the connection is accepted and returned from
The code above is creating our abstraction -
QuicConnectionContext
- twice: once in the callback and then again afterAcceptConnectionAsync
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 onQuicConnection
, and then use that value whenAcceptConnectionAsync
returns.Other options include adding
QuicConnectionContext
to aConditionalWeakTable<QuicConnection, QuicConnectionContext>
inside the callback, then removing it outside the callback. But a place to stash state on theQuicConnection
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
API Usage
Alternative Designs
Add a state argument to
ConnectionOptionsCallback
callback andAcceptConnectionAsync
.Risks
No response