Additional QuicConnectionOptions by rzikm · Pull Request #94211 · dotnet/runtime

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.

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

 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.

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

- 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

 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-analysisBot mentioned this pull request Oct 31, 2023
Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488
Closed

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 96dccbfCompare 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-analysisBot 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 teamNovember 16, 2023 14:27

build-analysisBot 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-analysisBot 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:mainNov 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-actionsBot locked and limited conversation to collaborators Dec 30, 2023

karelz added this to the 9.0.0 milestone May 14, 2024

Uh oh!

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-pipelinesBot commented Nov 16, 2023

Uh oh!

CarnaViireNov 22, 2023

Choose a reason for hiding this comment

Uh oh!

rzikmNov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ManickaPNov 27, 2023

Choose a reason for hiding this comment

Uh oh!

rzikmNov 27, 2023

Choose a reason for hiding this comment

Uh oh!

rzikmNov 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!

ManickaPNov 27, 2023

Choose a reason for hiding this comment

Uh oh!

ManickaPNov 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