QUIC stream limits #52704

ManickaP · 2021-05-13T14:42:23Z

Implements the 3rd option Allowing the caller to perform their own wait from #32079 (comment)

Adds WaitForAvailable(Bidi|Uni)rectionalStreamsAsync:
- triggered by peer announcement about new streams (QUIC_CONNECTION_EVENT_TYPE.STREAMS_AVAILABLE)
- if the connection is closed/disposed, the method throws QuicConnectionAbortedException which fitted our H3 better than boolean (can be changed)
~~Makes Open(Bidi|Uni)directionalStreamAsync:~~
- ~~asynchronous, awaiting msquic event QUIC_STREAM_EVENT_TYPE.START_COMPLETE~~
- ~~throwing if there's no stream capacity QUIC_STREAM_START_FLAGS.FAIL_BLOCKED~~
- ~~immediately notifying the peer about stream being opened QUIC_STREAM_START_FLAGS.IMMEDIATE~~
Changes stream limit type to int

Fixes #32079

Note: working on adding more tests, opening PR to kick off discussion.

…lue.

dotnet-issue-labeler · 2021-05-13T14:42:27Z

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, to 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 · 2021-05-13T14:42:30Z

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

Issue Details

Implements the 3rd option Allowing the caller to perform their own wait from #32079 (comment)

Adds WaitForAvailable(Bidi|Uni)rectionalStreamsAsync:
- triggered by peer announcement about new streams (QUIC_CONNECTION_EVENT_TYPE.STREAMS_AVAILABLE)
- if the connection is closed/disposed, the method throws QuicConnectionAbortedException which fitted our H3 better than boolean (can be changed)
Makes Open(Bidi|Uni)directionalStreamAsync:
- asynchronous, awaiting msquic event QUIC_STREAM_EVENT_TYPE.START_COMPLETE
- throwing if there's no stream capacity QUIC_STREAM_START_FLAGS.FAIL_BLOCKED
- immediately notifying the peer about stream being opened QUIC_STREAM_START_FLAGS.IMMEDIATE
Changes stream limit type to int

Fixes #32079

Note: working on adding more tests, opening PR to kick off discussion.

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

geoffkizer · 2021-05-22T23:39:49Z

immediately notifying the peer about stream being opened QUIC_STREAM_START_FLAGS.IMMEDIATE

Why do this? Seems like this would be bad for H3 perf.

geoffkizer · 2021-05-22T23:44:43Z

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

-        public long GetRemoteAvailableUnidirectionalStreamCount() { throw null; }
-        public System.Net.Quic.QuicStream OpenBidirectionalStream() { throw null; }
-        public System.Net.Quic.QuicStream OpenUnidirectionalStream() { throw null; }
+        public int GetRemoteAvailableBidirectionalStreamCount() { throw null; }


Why was this changed? Is it limited to int.Max by QUIC itself?

Msquic returns ushort:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

Lines 338 to 346 in 51b6188

internal override long GetRemoteAvailableUnidirectionalStreamCount()

{

return MsQuicParameterHelpers.GetUShortParam(MsQuicApi.Api, _state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_UNIDI_STREAM_COUNT);

}

internal override long GetRemoteAvailableBidirectionalStreamCount()

{

return MsQuicParameterHelpers.GetUShortParam(MsQuicApi.Api, _state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_BIDI_STREAM_COUNT);

}

AFAIK, we usually go with signed types for APIs ==> int.

I'm aware that QUIC sends long-range values in MAX_STREAMS frame, but that's cumulative number of streams which msquic hides from us. This is probably the original motivation for the long type.

Once again, I'm not against keeping it long. We discussed it briefly in one of our meetings and the consensus was for int so I went ahead with the change.

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

...m.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/ResettableCompletionSource.cs

ManickaP · 2021-05-26T17:09:08Z

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

@@ -53,6 +53,56 @@ public async Task UnidirectionalAndBidirectionalChangeValues()
            Assert.Equal(20, serverConnection.GetRemoteAvailableUnidirectionalStreamCount());
        }

+        [Fact]
+        [ActiveIssue("https://github.com/dotnet/runtime/issues/52048")]


These 2 new tests pass on their own but if run within the whole suite they cause the crash with stream being finalized after the connection. That should be fixed with #52800, I'll revisit it enabling them afterwards.

We discussed offline that it is caused by not accepted stream getting stuck inside accept queue and not being disposed there. You can try accepting it (after all test checks if it's important for the test) and disposing manually, or you may wait for Tomas' fix, your choice.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

src/libraries/System.Net.Http/src/System.Net.Http.csproj

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs

CarnaViire · 2021-06-03T16:41:04Z

LGTM, waiting on test cleanup and resolving a question about two locks vs one

ManickaP · 2021-06-03T17:51:20Z

I moved the experiments with the failing test into its own branch and disabled it in this PR (with an ActiveIssue).
Everything should be cleaned up a addressed now. Only waiting for a final review.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

CarnaViire

LGTM, but it would be great if @scalablecory or @geoffkizer could also take a look.

ManickaP added 7 commits May 6, 2021 12:17

Stream limits are ints since they cannot be greater than ushort.MaxVa…

9b107cd

…lue.

Stream opening APIs made async.

fae4adc

Proper implementation of OpenStreamAsync, waiting for the msquic event.

f74915e

WaitForAvailable*StreamsAsync

8e92adb

WaitForAvailable*StreamsAsync implementation

dbb626d

WaitForAvailable*StreamsAsync usage in H3

20a5eee

WaitForAvailable*StreamsAsync usage in H3 - fixed existing tests

fdc4575

ManickaP requested review from CarnaViire, scalablecory, geoffkizer and wfurt May 13, 2021 14:42

dotnet-issue-labeler bot added area-System.Net.Quic new-api-needs-documentation labels May 13, 2021

ManickaP added 4 commits May 13, 2021 20:06

Added and fixed H/3 test.

d9c087d

Merge branch 'main' into mapichov/32079_stream_limit

622aa8f

Post merge code fix

30efc26

Added some more tests and fixed token source

47358ef

ManickaP mentioned this pull request May 17, 2021

keep MsQuicConnection alive when streams are pending #52800

Merged

runfoapp bot mentioned this pull request May 20, 2021

InvokeCodeThatShouldFirEvents_EnsureEventsFired fails on OSX #52710

Closed

ManickaP added 4 commits May 20, 2021 21:55

Fixed mock implementation and fixed completion source in msquic one.

ea92437

Merge branch 'main' into mapichov/32079_stream_limit

36567a0

Fixed active stream counting in mock connection.

04c559d

Refactored stream limit work in mocks.

6bd9ea7

geoffkizer reviewed May 22, 2021

View reviewed changes

src/libraries/System.Net.Quic/ref/System.Net.Quic.cs Show resolved Hide resolved

geoffkizer reviewed May 22, 2021

View reviewed changes

src/libraries/System.Net.Quic/ref/System.Net.Quic.cs Outdated Show resolved Hide resolved

geoffkizer reviewed May 22, 2021

View reviewed changes

...m.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/ResettableCompletionSource.cs Outdated Show resolved Hide resolved

Merge branch 'main' into mapichov/32079_stream_limit

910ab9f

ManickaP commented May 26, 2021

View reviewed changes

ManickaP requested a review from geoffkizer May 27, 2021 08:30

CarnaViire reviewed May 27, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs Outdated Show resolved Hide resolved

CarnaViire reviewed May 27, 2021

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs Outdated Show resolved Hide resolved

ManickaP force-pushed the mapichov/32079_stream_limit branch from ff29d76 to df0d529 Compare May 28, 2021 14:47

Fixed access to _connection only under lock.

97c4e53

ManickaP force-pushed the mapichov/32079_stream_limit branch 2 times, most recently from 653c080 to 6609348 Compare June 1, 2021 17:38

Merge branch 'main' into mapichov/32079_stream_limit

97b3185

ManickaP force-pushed the mapichov/32079_stream_limit branch 2 times, most recently from 544bea4 to cabc0eb Compare June 2, 2021 15:43

CarnaViire reviewed Jun 3, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System.Net.Http.csproj Outdated Show resolved Hide resolved

CarnaViire reviewed Jun 3, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs Outdated Show resolved Hide resolved

ManickaP force-pushed the mapichov/32079_stream_limit branch from cabc0eb to 719df10 Compare June 3, 2021 14:28

Disabled one misbehaving test.

cbdd1df

ManickaP mentioned this pull request Jun 3, 2021

#52704 spin-off for testing failing CI #53683

Closed

ManickaP force-pushed the mapichov/32079_stream_limit branch from 719df10 to cbdd1df Compare June 3, 2021 17:32

ManickaP mentioned this pull request Jun 3, 2021

[QUIC] SendMoreThanStreamLimitRequestsConcurrently_LastWaits on Mocks hangs in CI #53688

Closed

CarnaViire reviewed Jun 3, 2021

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs Outdated Show resolved Hide resolved

CarnaViire approved these changes Jun 3, 2021

View reviewed changes

Addressed feedback.

7eb7fb1

ManickaP force-pushed the mapichov/32079_stream_limit branch from 9ea99fa to 7eb7fb1 Compare June 4, 2021 18:20

ManickaP merged commit a822d39 into dotnet:main Jun 6, 2021

ManickaP deleted the mapichov/32079_stream_limit branch June 6, 2021 11:08

ManickaP mentioned this pull request Jun 7, 2021

H/3 stream limit fix #53801

Merged

ghost locked as resolved and limited conversation to collaborators Jul 6, 2021

karelz added this to the 6.0.0 milestone Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC stream limits #52704

QUIC stream limits #52704

ManickaP commented May 13, 2021 •

edited

Loading

dotnet-issue-labeler bot commented May 13, 2021

ghost commented May 13, 2021

geoffkizer commented May 22, 2021

geoffkizer May 22, 2021

ManickaP May 24, 2021 •

edited

Loading

ManickaP May 26, 2021

CarnaViire May 27, 2021

CarnaViire commented Jun 3, 2021

ManickaP commented Jun 3, 2021

CarnaViire left a comment

	internal override long GetRemoteAvailableUnidirectionalStreamCount()
	{
	return MsQuicParameterHelpers.GetUShortParam(MsQuicApi.Api, _state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_UNIDI_STREAM_COUNT);
	}

	internal override long GetRemoteAvailableBidirectionalStreamCount()
	{
	return MsQuicParameterHelpers.GetUShortParam(MsQuicApi.Api, _state.Handle, QUIC_PARAM_LEVEL.CONNECTION, (uint)QUIC_PARAM_CONN.LOCAL_BIDI_STREAM_COUNT);
	}

QUIC stream limits #52704

QUIC stream limits #52704

Conversation

ManickaP commented May 13, 2021 • edited Loading

dotnet-issue-labeler bot commented May 13, 2021

ghost commented May 13, 2021

geoffkizer commented May 22, 2021

geoffkizer May 22, 2021

Choose a reason for hiding this comment

ManickaP May 24, 2021 • edited Loading

Choose a reason for hiding this comment

ManickaP May 26, 2021

Choose a reason for hiding this comment

CarnaViire May 27, 2021

Choose a reason for hiding this comment

CarnaViire commented Jun 3, 2021

ManickaP commented Jun 3, 2021

CarnaViire left a comment

Choose a reason for hiding this comment

ManickaP commented May 13, 2021 •

edited

Loading

ManickaP May 24, 2021 •

edited

Loading