Additional QuicConnectionOptions#94211

rzikm · 2023-10-31T13:27:20Z

Adds:

KeepAliveInterval - sending ping frames to prevent the connection from idling out. (we may want to add the same property on QuicConnection itself so that users can turn pings on/off according to their needs)
flow-control limits (max_data) - stream-level limits will have effect only with newer MsQuic having Support setting flow control limits for individual stream types microsoft/msquic#3948. 2.4+ should be it
HandshakeTimeout - kills the connection if the handshake is not established in the given amount of time
- ServerOptionsCallback still has 10s timeout as before, once the callback returns, the timeout value from the options is applied.
  The client times out as well.
- Timeouts lead to peer receiving UserCanceled TLS Alert which is weird but is not against the spec, although getting CONNECTION_CLOSE with APPLICATION_ERROR would be more expected.

Other improvements:

if the client terminates the connection during the handshake, CancellationToken passed to ServerOptionsCallback fires as well.
Fix property names in ArgumentExceptions thrown from connection options validation (previously it put the message instead of the parameter name)

ghost · 2023-10-31T13:27:28Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-10-31T13:27:35Z

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

Issue Details

Closes #72984.

Draft for now until we go through API review.

Author:	rzikm
Assignees:	-
Labels:	`new-api-needs-documentation`, `area-System.Net.Quic`
Milestone:	-

ManickaP

I know it's a draft, but I might not have time to review this properly later.

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

rzikm · 2023-11-16T14:27:38Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2023-11-16T14:27:49Z

Azure Pipelines successfully started running 1 pipeline(s).

CarnaViire · 2023-11-22T21:25:08Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

    private readonly ValueTaskSource _connectedTcs = new ValueTaskSource();
    private readonly ValueTaskSource _shutdownTcs = new ValueTaskSource();

+    private readonly CancellationTokenSource _shutdownCancellationTokenSource = new CancellationTokenSource();


I think we might need to dispose _shutdownCancellationTokenSource?
Also, the naming confuses me... It implies the cancellation of the connection's shutdown, but it's not, it's for cancelling the handshake in the Listener, right? (and it also means it's not needed on the client side?)

It is used only for listener to cancel the connection options callback if peer closes the connection before the handshake. I did not find a way to put the CTS outside QuicConnection.

I will try to come up with a better name

What do we do in SslStream case? Do we also react and cancel user callback when the client kills handshake in the meantime?

With sslstream we don't check for client-side activity while invoking the callback. We would have to always have a pending read on the inner stream and check for errors there. SslStream passes in the same token as to AuthenticateAsClientAsync.

Renamed to ConnectionShutdownToken and added a comment

ManickaP

In general looks good. I think the last contentious thing is the _shutdownCancellationTokenSource.

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnectionOptions.cs

ManickaP · 2023-11-27T11:05:20Z

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs

-            Exception exception = await AssertThrowsQuicExceptionAsync(QuicError.ConnectionTimeout, async () => await listener.AcceptConnectionAsync());
-            Assert.Equal(SR.Format(SR.net_quic_handshake_timeout, QuicDefaults.HandshakeTimeout), exception.Message);
+            QuicClientConnectionOptions clientOptions = CreateQuicClientOptions(listener.LocalEndPoint);
+            clientOptions.HandshakeTimeout = clientShorterTimeout ? TimeSpan.FromSeconds(2) : TimeSpan.FromSeconds(20);


I'm pretty sure this will eventually pop-up in CI report. Especially if you're asserting specific types of exception that should happen only in certain order of events depending on this timing.

But we can wait until we see it.

ManickaP · 2023-11-27T11:09:13Z

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

    private readonly ValueTaskSource _connectedTcs = new ValueTaskSource();
    private readonly ValueTaskSource _shutdownTcs = new ValueTaskSource();

+    private readonly CancellationTokenSource _shutdownCancellationTokenSource = new CancellationTokenSource();


What do we do in SslStream case? Do we also react and cancel user callback when the client kills handshake in the meantime?

ManickaP

Thank you!

rzikm · 2023-11-28T13:56:57Z

All CI failures are known build failures

ManickaP · 2023-11-28T14:57:58Z

BTW, I assume this also fixes #70090

CarnaViire

LGTM, thanks

ghost added area-System.Net.Quic new-api-needs-documentation labels Oct 31, 2023

ghost assigned rzikm Oct 31, 2023

build-analysis bot mentioned this pull request Oct 31, 2023

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

rzikm added 3 commits November 15, 2023 11:14

[API Proposal]: Additional QuicConnectionOptions

92bcf06

Comment change

d26aab8

Reflect latest API review

96dccbf

rzikm force-pushed the 72984-API-Proposal-Additional-QuicConnectionOptions branch from 8791161 to 96dccbf Compare November 15, 2023 12:42

rzikm added 2 commits November 15, 2023 18:27

Use HandshakeTimeout

4411b52

Use Connect timeout on client as well

6d34b07

build-analysis bot mentioned this pull request Nov 15, 2023

Tracking issue for CI build timeouts #76454

Closed

ManickaP reviewed Nov 16, 2023

View reviewed changes

rzikm added 2 commits November 16, 2023 14:01

Code review feedback

0ae7841

Cancel ServerOptionsCallback when client closes connection

dee37af

rzikm marked this pull request as ready for review November 16, 2023 14:27

rzikm requested a review from a team November 16, 2023 14:27

build-analysis bot mentioned this pull request Nov 16, 2023

System.ComponentModel.Win32Exception : An error occurred trying to start process 'notepad.exe' with working directory 'C:\h\w\9F8D0903\w\B7E309C0\e'. Access is denied #90406

Closed

3 tasks

CarnaViire reviewed Nov 22, 2023

View reviewed changes

ManickaP reviewed Nov 27, 2023

View reviewed changes

Code review feedback

f666277

build-analysis bot mentioned this pull request Nov 28, 2023

Failing test -System.IO.Tests.File_GetSetTimes_SafeFileHandle_Pathless.WritingShouldUpdateWriteTime_After_SetLastAccessTime on windows/x86 #95298

Closed

ManickaP approved these changes Nov 28, 2023

View reviewed changes

rzikm requested a review from CarnaViire November 28, 2023 13:57

CarnaViire approved these changes Nov 28, 2023

View reviewed changes

rzikm merged commit 563664e into dotnet:main Nov 29, 2023

ManickaP mentioned this pull request Nov 29, 2023

[HTTP3] [outerloop] Requesting HTTP version 3.0 with version policy RequestVersionExact while unable to establish HTTP/3 connection #95413

Closed

bentoi mentioned this pull request Dec 27, 2023

Missing Quic keep alive option to enable keep alive icerpc/icerpc-csharp#3353

Closed

github-actions bot locked and limited conversation to collaborators Dec 30, 2023

karelz added this to the 9.0.0 milestone May 14, 2024

Conversation

rzikm commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 31, 2023

Uh oh!

ghost commented Oct 31, 2023

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rzikm commented Nov 16, 2023

Uh oh!

azure-pipelines bot commented Nov 16, 2023

Uh oh!

CarnaViire Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

rzikm Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ManickaP Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

rzikm Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

rzikm Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ManickaP Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ManickaP Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

rzikm commented Nov 28, 2023

Uh oh!

ManickaP commented Nov 28, 2023

Uh oh!

CarnaViire left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rzikm commented Oct 31, 2023 •

edited

Loading