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

[API Proposal]: Additional QuicConnectionOptions #72984

Closed
JamesNK opened this issue Jul 28, 2022 · 12 comments · Fixed by #94211
Closed

[API Proposal]: Additional QuicConnectionOptions #72984

JamesNK opened this issue Jul 28, 2022 · 12 comments · Fixed by #94211
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Quic
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented Jul 28, 2022

Background and motivation

msquic has a lot of connection options and S.N.Q exposes just a few

List of msquic settings - https://github.com/microsoft/msquic/blob/main/docs/Settings.md

Settings I think should be added:

  • Disconnect Timeout - Importance discovered as part of QUIC: QuicStream.WaitForWriteCompletionAsync sometimes doesn't complete #71927. People may want to be more or less aggressive with closing unresponsive streams
  • Keep Alive Interval - Kestrel HTTP/2 layer has a similar option. Should expose it for feature parity.
  • Flow Control Window - Kestrel HTTP/2 layer has a similar option. Should expose it for feature parity. Useful when sending large amounts of data over connection with latency. Allows you to trade potentially higher memory usage to avoid back pressure throttling the stream/connection
  • Stream Receive Window - Kestrel HTTP/2 layer has a similar option. Same reason as connection flow control window.
    • ncl: QUIC exposes limits for individual stream types, so we will expose separate properties for those
  • DatagramReceiveEnabled - Maybe. I think this will be needed for WebTransport datagram support. Does QuicStream have a story for supporting datagram in the future?
  • Handshake Idle Timeout - Kestrel has an option to supply a handshake timeout. Would be nice to flow that through to QUIC.

I don't think any of these are necessary for .NET 7.

API Proposal (edited by @rzikm)

See also discussion about naming below the proposal, we are open for suggestions for better names. Default values were copied from current MsQuic behavior.

namespace System.Net.Quic
{
    public abstract class QuicConnectionOptions
    {
        /// <summary>
        /// The interval at which keep alive packets are sent on the connection.
        /// Default <see cref="TimeSpan.Zero"/>, or <see cref="Timeout.InfiniteTimeSpan"> means no keep alive.
        /// </summary>
+       public TimeSpan KeepAliveInterval { get; set; } = TimeSpan.Zero;

        /// <summary>
        /// The initial flow-control window size for the connection.
        /// </summary>
+       public int InitialConnectionReceiveWindowSize { get; set; } = 16 * 1024 * 1024;

        /// <summary>
        /// The initial flow-control window size for locally initiated bidirectional streams.
        /// </summary>
+       public int InitialLocallyInitiatedBidirectionalStreamReceiveWindowSize { get; set; } = 64 * 1024;

        /// <summary>
        /// The initial flow-control window size for remotely initiated bidirectional streams.
        /// </summary>
+       public int InitialRemotelyInitiatedBidirectionalStreamReceiveWindowSize { get; set; } = 64 * 1024;

        /// <summary>
        /// The initial flow-control window size for (remotely initiated) unidirectional streams.
        /// </summary>
+       public int InitialUnidirectionalStreamReceiveWindowSize { get; set; } = 64 * 1024;

        /// <summary>
        /// The upper bound on time when the handshake must complete. If the handshake does not
        /// complete in this time, the connection is aborted.
        /// </summary>
+       public TimeSpan HandshakeTimeout { get; set; } = TimeSpan.FromSeconds(10);
    }
}

Properties InitialConnectionReceiveWindowSize, InitialLocallyInitiatedBidirectionalStreamReceiveWindowSize, InitialRemotelyInitiatedBidirectionalStreamReceiveWindowSize, and InitialUnidirectionalStreamReceiveWindowSize have rather long names but we did come up with a better alternative. They map directly to QUIC transport parameters initial_max_data, initial_max_stream_data_bidi_local, initial_max_stream_data_bidi_remote, and initial_max_stream_data_uni respectively. These transport parameters limit the amount of data the peer can send on the connection as a whole and on streams of individual types to prevent the peer from overwhelming the receiver with data. Thus, the Initial*ReceiveWindowSize part of the name seems appropriate. The rest of the name identifies the type of stream, where "Locally initiated bidirectional stream" and similar are terms used in the QUIC RFC and we should use them to avoid confusion.

For comparison, MsQuic uses names such as StreamRecvWindowBidiLocalDefault.
And will expand the receive window if appropriate.

Parameter definition from RFC

initial_max_data (0x04)

The initial maximum data parameter is an integer value that contains the
initial value for the maximum amount of data that can be sent on the
connection. This is equivalent to sending a MAX_DATA (Section
19.9
) for the
connection immediately after completing the handshake.

initial_max_stream_data_bidi_local (0x05)

This parameter is an integer value specifying the initial flow control limit
for locally initiated bidirectional streams. This limit applies to newly
created bidirectional streams opened by the endpoint that sends the
transport parameter. In client transport parameters, this applies to streams
with an identifier with the least significant two bits set to 0x00; in
server transport parameters, this applies to streams with the least
significant two bits set to 0x01.

initial_max_stream_data_bidi_remote (0x06)

This parameter is an integer value specifying the initial flow control limit
for peer-initiated bidirectional streams. This limit applies to newly
created bidirectional streams opened by the endpoint that receives the
transport parameter. In client transport parameters, this applies to streams
with an identifier with the least significant two bits set to 0x01; in
server transport parameters, this applies to streams with the least
significant two bits set to 0x00.

initial_max_stream_data_uni (0x07)

This parameter is an integer value specifying the initial flow control limit
for unidirectional streams. This limit applies to newly created
unidirectional streams opened by the endpoint that receives the transport
parameter. In client transport parameters, this applies to streams with an
identifier with the least significant two bits set to 0x03; in server
transport parameters, this applies to streams with the least significant two
bits set to 0x02.

API Usage

var quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionCallbackOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (connection, helloInfo, cancellationToken) =>
    {
        var connectionOptions = new QuicServerConnectionOptions
        {
            // ... set various properties for server connection ...
        };
        return connectionOptions;
    }
};

Alternative Designs

To avoid long property names. We can introduce a wrapper type for the initial
connection window sizes. However, the wrapper type must be a class, otherwise
options.InitialReceiveWindowSizes.ConnectionTotal = x has no effect. We also
don't see another use for the type.

namespace System.Net.Quic
{
+   public class QuicReceiveWindowSizes
    {
        /// <summary>
        /// The flow-control window size for the entire connection.
        /// </summary>
+       public int ConnectionTotal { get; set; } = 16 * 1024 * 1024;

        /// <summary>
        /// The flow-control window size for locally initiated bidirectional streams.
        /// </summary>
+       public int LocallyInitiatedBidirectionalStreams { get; set; } = 64 * 1024;

        /// <summary>
        /// The flow-control window size for remotely initiated bidirectional streams.
        /// </summary>
+       public int RemotelyInitiatedBidirectionalStreams { get; set; } = 64 * 1024;

        /// <summary>
        /// The flow-control window size for (remotely initiated) unidirectional streams.
        /// </summary>
+       public int UnidirectionalStreams { get; set; } = 64 * 1024;

    }

    public abstract class QuicConnectionOptions
    {
        // unchanged
+       public TimeSpan KeepAliveInterval { get; set; } = TimeSpan.Zero;
+       public TimeSpan HandshakeTimeout { get; set; } = TimeSpan.FromSeconds(10);

        /// <summary>
        /// The initial connection window sizes.
        /// Default <see cref="TimeSpan.Zero"/>, or <see cref="Timeout.InfiniteTimeSpan"> means no keep alive.
        /// </summary>
        // note: actuall implementation can create the default instance lazily to avoid alloc if user only calls set
+       public QuicReceiveWindowSizes InitialReceiveWindowSizes { get; set; } = new QuicReceiveWindowSizes();
    }
}

Risks

The options above are commonly exposed by other QUIC implementations and none of them are too specific as to vendor lock System.Net.Quic to MsQuic only.

@JamesNK JamesNK added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http labels Jul 28, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 28, 2022
@ghost
Copy link

ghost commented Jul 28, 2022

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

Issue Details

Background and motivation

msquic has a lot of connection options and S.N.Q exposes just a few

List of msquic settings - https://github.com/microsoft/msquic/blob/main/docs/Settings.md

Settings I think should be added:

  • DisconnectTimeout - Importance discovered as part of QUIC: QuicStream.WaitForWriteCompletionAsync sometimes doesn't complete #71927. People may want to be more or less aggressive with closing unresponsive streams
  • KeepAliveInterval - Kestrel HTTP/2 layer has a similar option. Should expose it for feature parity.
  • Flow Control Window - Kestrel HTTP/2 layer has a similar option. Should expose it for feature parity. Useful to trade potentially higher memory usage to avoid back pressure
  • Stream Receive Window - Kestrel HTTP/2 layer has a similar option. Same reason as flow control window.
  • DatagramReceiveEnabled - Maybe. I think this will be needed for WebTransport datagram support. Does QuicStream have a story for supporting datagram in the future?
  • Handshake Idle Timeout - Kestrel has an option to supply a handshake timeout. Would be nice to flow that through to QUIC.

I don't think any of these are necessary for .NET 7.

API Proposal

namespace System.Net.Quic
{
    public abstract class QuicConnectionOptions
    {
        public TimeSpan DisconnectTimeout { get; set; }
        public int InitialConnectionWindowSize { get; set; }
        public int InitialStreamWindowSize { get; set; }
        public TimeSpan KeepAliveInterval { get; set; }
        public bool DatagramReceiveEnabled { get; set; }
        public TimeSpan HandshakeTimeout { get; set; }
    }
}

API Usage

var quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionCallbackOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (connection, helloInfo, cancellationToken) =>
    {
        var connectionOptions = new QuicServerConnectionOptions
        {
            // ... set various properties for server connection ...
        };
        return connectionOptions;
    }
};

Alternative Designs

No response

Risks

No response

Author: JamesNK
Assignees: -
Labels:

api-suggestion, area-System.Net.Http

Milestone: -

@rzikm
Copy link
Member

rzikm commented Jul 28, 2022

All proposed options seem to be supported by MsQuic

Note that DatagramReceiveEnabled will need more API design, related issue is #53533

@CarnaViire
Copy link
Member

That seems reasonable, but given it's not critical for 7.0, I'll put it to Future for now

@CarnaViire CarnaViire added this to the Future milestone Jul 28, 2022
@CarnaViire CarnaViire removed the untriaged New issue has not been triaged by the area owner label Jul 28, 2022
@ManickaP
Copy link
Member

Triage: we should revisit this in 8.0. We should check usefulness of the parameters and whether they make sense outside of msquic context, e.g. are they transport parameters from the spec?

@ghost
Copy link

ghost commented Jun 9, 2023

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

Issue Details

Background and motivation

msquic has a lot of connection options and S.N.Q exposes just a few

List of msquic settings - https://github.com/microsoft/msquic/blob/main/docs/Settings.md

Settings I think should be added:

  • DisconnectTimeout - Importance discovered as part of QUIC: QuicStream.WaitForWriteCompletionAsync sometimes doesn't complete #71927. People may want to be more or less aggressive with closing unresponsive streams
  • KeepAliveInterval - Kestrel HTTP/2 layer has a similar option. Should expose it for feature parity.
  • Flow Control Window - Kestrel HTTP/2 layer has a similar option. Should expose it for feature parity. Useful when sending large amounts of data over connection with latency. Allows you to trade potentially higher memory usage to avoid back pressure throttling the stream/connection
  • Stream Receive Window - Kestrel HTTP/2 layer has a similar option. Same reason as connection flow control window.
  • DatagramReceiveEnabled - Maybe. I think this will be needed for WebTransport datagram support. Does QuicStream have a story for supporting datagram in the future?
  • Handshake Idle Timeout - Kestrel has an option to supply a handshake timeout. Would be nice to flow that through to QUIC.

I don't think any of these are necessary for .NET 7.

API Proposal

namespace System.Net.Quic
{
    public abstract class QuicConnectionOptions
    {
        public TimeSpan DisconnectTimeout { get; set; }
        public int InitialConnectionWindowSize { get; set; }
        public int InitialStreamWindowSize { get; set; }
        public TimeSpan KeepAliveInterval { get; set; }
        public bool DatagramReceiveEnabled { get; set; }
        public TimeSpan HandshakeTimeout { get; set; }
    }
}

API Usage

var quicListenerOptions = new QuicListenerOptions
{
    ApplicationProtocols = _tlsConnectionCallbackOptions.ApplicationProtocols,
    ListenEndPoint = listenEndPoint,
    ListenBacklog = options.Backlog,
    ConnectionOptionsCallback = async (connection, helloInfo, cancellationToken) =>
    {
        var connectionOptions = new QuicServerConnectionOptions
        {
            // ... set various properties for server connection ...
        };
        return connectionOptions;
    }
};

Alternative Designs

No response

Risks

No response

Author: JamesNK
Assignees: -
Labels:

api-suggestion, area-System.Net.Quic

Milestone: 8.0.0

@rzikm
Copy link
Member

rzikm commented Oct 3, 2023

Looking at the options more closely, with optics that we don't want to necessarily get locked to MsQuic and expose only options which will not prevent us to switch implementations in the future.

DisconnectTimeout

set in millisecond resolution, default value 16000ms. Does not map to a transport parameter, nor does RFC require this to be app-configurable. This setting seems to be specific to MsQuic.

KeepAliveInterval

disabled by default, set in millisecond resolution. RFC specifies that "An implementation of QUIC might provide applications with an option to defer an idle timeout." but does not specify the form. The other way to do that is providing a method which elicits sending ping manually (as Cloudflare's quiche does).

InitialConnectionWindowSize

This one is a bit tricky. There is InitialWindowPackets option which sets maximum number of packets in the initial congestion window. RFC gives a SHOULD recommendation of the value (which is default on MsQuic) and does not require the value to be app configurable. If we want to expose this as bytes, then our best effort is to divide the value by 1472 (common max MTU value of the L2 layer) to get approximate number of packets that translates to.

The other option is the ConnFlowControlWindow which is the connection-wide control flow window for the data sent by the peer, but AFAICT this does not change over the lifetime of the connection in MsQuic (although RFC does not prevent so). (default 16 MB). Maps directly to the initial_max_data transport parameter.

InitialStreamWindowSize

MsQuic option StreamRecvWindowDefault, I suggest renaming to InitialStreamReceiveWindowSize. Units are bytes.

however, QUIC as a protocol allows setting separate flow-control limits for unidirectional/bidirectional streams (and in bidirectional streams, separate values for the streams initiated locally and remotely). MsQuic just happens to use the same value for all 3 streams. This maps to initial_max_stream_data_* transport parameters.

RFC requires both connection-wide and stream-level control flow limits to be app-configurable.

HandshakeIdleTimeout

Set in millisecond resolution, the default value is 10s. This seems to be MsQuic-specific as I can't find anything about separate timeout for handshake in the RFC.

DatagramReceiveEnabled

to be left out from this proposal, it needs a separate API proposal which includes methods for actually sending/receiving datagrams (#53533)

@rzikm
Copy link
Member

rzikm commented Oct 5, 2023

We discussed this with the team, and we looked at other implementations to make sure we don't expose something that is not provided by other QUIC implementations if we ever decide to switch or provide option to use different implementation than MsQuic.

We found that all implementations expose similar configurations options except the DisconnectTimeout. And instead of having one InitialStreamWindowSize, most implementations (except MsQuic) expose limits for individual stream types (mapping to the three transport parameters). So we will follow up with MsQuic team if they are open to exposing it this way as well. Cc: @JamesNK.

@rzikm
Copy link
Member

rzikm commented Oct 5, 2023

Filed microsoft/msquic#3897 for separate options for stream-level flow control

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 31, 2023
@rzikm
Copy link
Member

rzikm commented Nov 6, 2023

Updated propsal based on discussion within the team. We have consensus on current proposal.

@rzikm rzikm added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Nov 6, 2023
@bartonjs
Copy link
Member

bartonjs commented Nov 14, 2023

Video

  • A brief discussion of making KeepAliveInterval nullable, but the consensus was to expose the Inifinite default.
  • We factored out the InitialLocallyInitiatedBidirectionalStreamReceiveWindowSize (et al) properties into a struct both for togetherness and to reduce the name
namespace System.Net.Quic
{
    public struct QuicReceiveWindowSizes
    {
        /// <summary>
        /// The initial flow-control window size for the connection.
        /// </summary>
        public int Connection { get; set; } = 16 * 1024 * 1024;

        /// <summary>
        /// The initial flow-control window size for locally initiated bidirectional streams.
        /// </summary>
        public int LocallyInitiatedBidirectionalStream { get; set; } = 64 * 1024;

         /// <summary>
        /// The initial flow-control window size for remotely initiated bidirectional streams.
        /// </summary>
        public int RemotelyInitiatedBidirectionalStream { get; set; } = 64 * 1024;

        /// <summary>
        /// The initial flow-control window size for (remotely initiated) unidirectional streams.
        /// </summary>
        public int UnidirectionalStream { get; set; } = 64 * 1024;
    }

    public partial class QuicConnectionOptions
    {
        /// <summary>
        /// The interval at which keep alive packets are sent on the connection.
        /// Default <see cref="TimeSpan.Zero"/>, or <see cref="Timeout.InfiniteTimeSpan"> means no keep alive.
        /// </summary>
        public TimeSpan KeepAliveInterval { get; set; } = TimeSpan.Infinite;

        public QuicReceiveWindowSizes InitialReceiveWindowSizes { get; set; }
        
        /// <summary>
        /// The upper bound on time when the handshake must complete. If the handshake does not
        /// complete in this time, the connection is aborted.
        /// </summary>
        public TimeSpan HandshakeTimeout { get; set; } = TimeSpan.FromSeconds(10);
    }
}

@bartonjs bartonjs removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 14, 2023
@bartonjs bartonjs added the api-approved API was approved in API review, it can be implemented label Nov 14, 2023
@rzikm
Copy link
Member

rzikm commented Nov 15, 2023

@bartonjs Defining QuicReceiveWindowSizes as struct poses some usability problems. Because now code like

options.InitialReceiveWindowSizes.Connection = x;

Does not compile because Cannot modify the return value of 'QuicClientConnectionOptions.InitialReceiveWindowSizes' because it is not a variable and workarounds are ugly.

Is it okay to change the type to class and make the QuicConnectionOptions.QuicReceiveWindowSizes always non-null by lazily allocating new instance if needed?

@bartonjs
Copy link
Member

Changing it to a class seems fine to me.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2023
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.Quic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants