[QUIC] Add QuicStream.WaitForWriteCompletionAsync #58236

JamesNK · 2021-08-27T02:16:22Z

dotnet-issue-labeler · 2021-08-27T02:16:26Z

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-08-27T02:16:29Z

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

Issue Details

Fixes #58229

Author:	JamesNK
Assignees:	-
Labels:	`area-System.Net.Quic`
Milestone:	-

ManickaP

Some small questions, otherwise looks good!
Thanks!

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockStream.cs

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

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

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs

ManickaP

LGTM, thanks!

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

CarnaViire · 2021-08-30T15:54:55Z

Just an FYI that I'm also looking at this, will post my review today

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

CarnaViire · 2021-08-30T16:22:11Z

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

@@ -1040,6 +1145,12 @@ private static uint HandleEventShutdownComplete(State state, ref StreamEvent evt

                shouldReadComplete = CleanupReadStateAndCheckPending(state, ReadState.ReadsCompleted);

+                if (state.ShutdownWriteState == ShutdownWriteState.None)


Are you sure this state is expected here? As you've mentioned in comment for HandleEventSendShutdownComplete, by the time we receive SHUTDOWN_COMPLETE event, we should either completed ShutdownWriteState already, or it is a connection close, which is handled separately in HandleEventConnectionClose. I think we may leave the logic here, but guard it with Debug.Assert... what do you think?

Even as a fallback, I think it is especially strange to complete it successfully here... IMO successful completion should only happen in HandleEventSendShutdownComplete.

There is one situation where you can get to this point without HandleEventSendShutdownComplete: if the stream is unidirectional and there are no writes.

Perhaps WaitForWriteCompleteAsync should error if it is called for this type of stream. I'm going to add that as a TODO comment. I'll also explain it in the logic inside HandleEventShutdownComplete. The exact behavior can be figured out in .NET 7.

CarnaViire · 2021-08-30T16:38:53Z

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

+                    Assert.Equal(1, received);
+                    Assert.True(serverStream.ReadsCompleted);
+
+                    long sendAbortErrorCode = await waitForAbortTcs.Task;


Could that be just

var ex = await Assert.ThrowsAsync<QuicStreamAbortedException>(() => serverStream.WaitForWriteCompletionAsync()); Assert.Equal(ExpectedErrorCode, ex.ErrorCode);

? (Here and below)

I want to test the wait part of WaitForWriteCompletionAsync

CarnaViire · 2021-08-30T16:43:56Z

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

+        [Fact]
+        public async Task WaitForWriteCompletionAsync_ClientReadAborted_Throws()
+        {
+            const int ExpectedErrorCode = 0xfffffff;


I wanted to comment that we use the same constant as a default error code on Dispose, but we actually use 0xffffffff, not 0xfffffff 😁

We might want to choose a different number, diff between 7x f and 8x f is too small to be noticeable.

Assert.Equal will show decimal representation, so it will be easier to distinguish... but it will be harder to catch in the future if some other test will accidentally use 8xf

I copied the value from a different test. IMO lets leave it as is for now, and if you want to change all instances of this value then it can be done in a future PR.

CarnaViire · 2021-08-30T17:23:31Z

I see that new tests are failing for Mock. Aborts didn't work properly on Mocks before, and I guess the changes didn't fix them fully... 😢 I wonder if we really want to invest into fixing Mock at this point. I would vote for no. Thoughts? @ManickaP @geoffkizer

ManickaP · 2021-08-30T17:33:37Z

I see that new tests are failing for Mock. Aborts didn't work properly on Mocks before, and I guess the changes didn't fix them fully... cry I wonder if we really want to invest into fixing Mock at this point.

We can disable the tests for mocks specifically, we have a precedent for that. So unless it's super obvious and easy to fix, I wouldn't put much effort in it.

geoffkizer · 2021-08-30T19:39:30Z

We can disable the tests for mocks specifically, we have a precedent for that. So unless it's super obvious and easy to fix, I wouldn't put much effort in it.

+1 to this

Once 6.0 is done we should discuss whether to keep Mock at all. But for now let's just disable specific test and not make any major changes.

JamesNK · 2021-08-31T02:38:41Z

We can disable the tests for mocks specifically, we have a precedent for that. So unless it's super obvious and easy to fix, I wouldn't put much effort in it.

It was super obvious and easy to fix.

JamesNK · 2021-08-31T02:40:12Z

All PR feedback has been applied. I'm creating a release/6.0 (RC2) backport issue with details about the change for @adityamandaleeka to take to tactics tomorrow.

Hopefully there are no more changes. If there is something then I can apply it to both PRs.

JamesNK · 2021-08-31T02:42:30Z

/backport to release/6.0

github-actions · 2021-08-31T02:42:42Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1184544908

karelz · 2021-08-31T08:52:36Z

Unrelated test failure (likely) #734 - see log

    System.Security.Cryptography.Rsa.Tests.SignVerify_Span.InvalidKeySize_DoesNotInvalidateKey [FAIL]
      Internal.Cryptography.CryptoThrowHelper+WindowsCryptographicException : Unknown error (0xc0000001)
      Stack Trace:
        /_/src/libraries/Common/src/Internal/Cryptography/CngCommon.SignVerify.cs(80,0): at Internal.Cryptography.CngCommon.TrySignHash(SafeNCryptKeyHandle keyHandle, ReadOnlySpan`1 hash, Span`1 signature, AsymmetricPaddingMode paddingMode, Void* pPaddingInfo, Int32& bytesWritten)
        /_/src/libraries/Common/src/System/Security/Cryptography/RSACng.SignVerify.cs(125,0): at System.Security.Cryptography.RSACng.TrySignHash(ReadOnlySpan`1 hash, Span`1 destination, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding, Int32& bytesWritten)
        /_/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RSA.cs(206,0): at System.Security.Cryptography.RSA.TrySignData(ReadOnlySpan`1 data, Span`1 destination, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding, Int32& bytesWritten)
        /_/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.netcoreapp.cs(11,0): at System.Security.Cryptography.Rsa.Tests.SignVerify_Span.<>c__DisplayClass0_0.<SignData>b__0(Byte[] dest)
        /_/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.netcoreapp.cs(27,0): at System.Security.Cryptography.Rsa.Tests.SignVerify_Span.TryWithOutputArray(Func`2 func)
        /_/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.netcoreapp.cs(11,0): at System.Security.Cryptography.Rsa.Tests.SignVerify_Span.SignData(RSA rsa, Byte[] data, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding)
        /_/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.cs(114,0): at System.Security.Cryptography.Rsa.Tests.SignVerify.InvalidKeySize_DoesNotInvalidateKey()

runtime-staging failures seem to be unrelated, rerunning.

JamesNK · 2021-08-31T10:14:04Z

/backport to release/6.0

github-actions · 2021-08-31T10:14:18Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1185710989

CarnaViire

LGTM, thanks!

karelz · 2021-08-31T21:17:17Z

dotnet-linker-tests failure - hit 120min timeout. Rerunning.

adityamandaleeka · 2021-08-31T23:43:35Z

Thanks everyone!

JamesNK added the area-System.Net.Quic label Aug 27, 2021

JamesNK requested review from CarnaViire, ManickaP and geoffkizer August 27, 2021 02:16

dotnet-issue-labeler bot added the new-api-needs-documentation label Aug 27, 2021

JamesNK mentioned this pull request Aug 27, 2021

[release/6.0] HTTP/3: Support canceling requests that aren't reading a body dotnet/aspnetcore#35823

Closed

runfoapp bot mentioned this pull request Aug 27, 2021

Feed unreliability affecting CI #55449

Closed

ManickaP reviewed Aug 27, 2021

View reviewed changes

kasperk81 reviewed Aug 27, 2021

View reviewed changes

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

geoffkizer reviewed Aug 27, 2021

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/Mock/MockConnection.cs Show resolved Hide resolved

ManickaP approved these changes Aug 30, 2021

View reviewed changes

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

CarnaViire reviewed Aug 30, 2021

View reviewed changes

Add QuicStream.WaitForWriteCompletionAsync

75be833

JamesNK force-pushed the jamesnk/quic-waitforwritecompletion branch from 7fdedf2 to 75be833 Compare August 31, 2021 02:42

github-actions bot mentioned this pull request Aug 31, 2021

[release/6.0] [QUIC] Add QuicStream.WaitForWriteCompletionAsync #58415

Merged

karelz mentioned this pull request Aug 31, 2021

Test failure: System.Security.Cryptography.X509Certificates.Tests.RevocationTests.DynamicRevocationTests.NothingRevoked #734

Closed

Fix flakey mock stream tests

c877465

karelz approved these changes Aug 31, 2021

View reviewed changes

CarnaViire approved these changes Aug 31, 2021

View reviewed changes

runfoapp bot mentioned this pull request Aug 31, 2021

STATUS_UNSUCCESSFUL in RsaCryptRoundtrip_OaepSHA1 #29683

Open

adityamandaleeka merged commit 629f60e into dotnet:main Aug 31, 2021

karelz added this to the 7.0.0 milestone Sep 1, 2021

karelz mentioned this pull request Sep 1, 2021

QUIC: Add API to allow detecting when peer aborts receive #58229

Closed

ghost locked as resolved and limited conversation to collaborators Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUIC] Add QuicStream.WaitForWriteCompletionAsync #58236

[QUIC] Add QuicStream.WaitForWriteCompletionAsync #58236

JamesNK commented Aug 27, 2021

dotnet-issue-labeler bot commented Aug 27, 2021

ghost commented Aug 27, 2021

ManickaP left a comment

ManickaP left a comment

CarnaViire commented Aug 30, 2021

CarnaViire Aug 30, 2021

CarnaViire Aug 30, 2021

JamesNK Aug 30, 2021

CarnaViire Aug 30, 2021

JamesNK Aug 30, 2021

CarnaViire Aug 30, 2021

ManickaP Aug 30, 2021

CarnaViire Aug 30, 2021

JamesNK Aug 30, 2021

CarnaViire commented Aug 30, 2021

ManickaP commented Aug 30, 2021

geoffkizer commented Aug 30, 2021

JamesNK commented Aug 31, 2021

JamesNK commented Aug 31, 2021 •

edited

Loading

JamesNK commented Aug 31, 2021

github-actions bot commented Aug 31, 2021

karelz commented Aug 31, 2021 •

edited

Loading

JamesNK commented Aug 31, 2021

github-actions bot commented Aug 31, 2021

CarnaViire left a comment

karelz commented Aug 31, 2021

adityamandaleeka commented Aug 31, 2021

		@@ -1040,6 +1145,12 @@ private static uint HandleEventShutdownComplete(State state, ref StreamEvent evt

		shouldReadComplete = CleanupReadStateAndCheckPending(state, ReadState.ReadsCompleted);

		if (state.ShutdownWriteState == ShutdownWriteState.None)

[QUIC] Add QuicStream.WaitForWriteCompletionAsync #58236

[QUIC] Add QuicStream.WaitForWriteCompletionAsync #58236

Conversation

JamesNK commented Aug 27, 2021

dotnet-issue-labeler bot commented Aug 27, 2021

ghost commented Aug 27, 2021

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

CarnaViire commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarnaViire commented Aug 30, 2021

ManickaP commented Aug 30, 2021

geoffkizer commented Aug 30, 2021

JamesNK commented Aug 31, 2021

JamesNK commented Aug 31, 2021 • edited Loading

JamesNK commented Aug 31, 2021

github-actions bot commented Aug 31, 2021

karelz commented Aug 31, 2021 • edited Loading

JamesNK commented Aug 31, 2021

github-actions bot commented Aug 31, 2021

CarnaViire left a comment

Choose a reason for hiding this comment

karelz commented Aug 31, 2021

adityamandaleeka commented Aug 31, 2021

JamesNK commented Aug 31, 2021 •

edited

Loading

karelz commented Aug 31, 2021 •

edited

Loading