Implement QUIC abort exceptions, fix channel usage, and implement some tests. #32051

scalablecory · 2020-02-10T21:15:37Z

New: implement QuicStreamAbortedException, QuicConnectionAbortedException, QuicOperationAbortedException.
Fix: MsQuic returning null implementation when completing listener/connection channels.
Fix: some flags types being defined as 8-bit when they are 32-bit in msquic.h
Reorganize tests a little bit and avoid a shared listener creating variability.
Add some additional tests demonstrating failures seen in HttpClient. Issues created for those: #32050 #32049 #32048

jkotalik · 2020-02-10T21:21:56Z

Fix: MsQuic returning null implementation when completing listener/connection channels.

I was just about to do this 😄

Reorganize tests a little bit and avoid a shared listener creating variability.

I thought the listener was created once per test still. Did you see this variability?

…tion, QuicOperationAbortedException. Fix: MsQuic returning null implementation when completing listener/connection channels. Fix: some flags types being defined as 8-bit when they are 32-bit in msquic.h Add some additional tests demonstrating failures seen in HttpClient.

scalablecory · 2020-02-10T22:22:52Z

some http3 code snuck into the commit; fixed now :)

scalablecory · 2020-02-10T22:29:47Z

I thought the listener was created once per test still. Did you see this variability?

Yes, xunit test classes are newed up once per test so it wasn't like a race condition. More for organization purposes (I'm worried about unintentionally using it in a way that exercises race conditions VS a specific test -- this is more explicit and in line with how most of our other tests function)

jkotalik · 2020-02-10T23:11:54Z

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

+                    shouldComplete = true;
+                }
+                _sendState = SendState.Aborted;
+                _sendErrorCode = evt.Data.PeerSendAbort.ErrorCode;


Make sendErrorCode a local variable. I don't believe it is used elsewhere.

It is used to throw a QuicStreamAbortedException on the other side of the ValueTask

jkotalik · 2020-02-10T23:13:23Z

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

@@ -21,11 +21,13 @@ public class MsQuicTests : MsQuicTestBase
        [Fact]
        public async Task BasicTest()
        {
+            using QuicListener listener = CreateQuicListener();


I think this is more of a style thing. I personally prefer initialization of the listener in the constructor, but I'm not strongly opposed to making it explicit

halter73 · 2020-02-11T00:51:25Z

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

            }

            if (NetEventSource.IsEnabled) NetEventSource.Exit(this);
-
-            return null;


I think it's better to continue returning null than to throw an exception from AcceptStreamAsync when the connection is closing. The connection being closed is a common enough that throwing an exception feels noisy. I know this will make the QuicConnection.AcceptStreamAsync() wrapper a little more complicated, but I think it's worth it.

Yeah, looking back on this, I agree as well.

I think that anyone familiar with sockets will expect consistency with that, which is why I went that direction, but I also think most everyone else might expect it will return null. The question becomes who is the more likely user. It's trouble to merge Stream and Socket 🙃.

I don't feel strongly about it. We can change it.

Let's keep it with the "current" behavior of returning null, but fix it in QuicConnection.AcceptStreamAsync.

Actually, I think this needs some more thought now. We should consider how consistent the APIs between QuicStream, QuicConnection, QuicListener need to be in how they behave on disposal. QuicStream.ReadAsync() etc. do not return a nullable type and will need to throw an exception.

I'm going to commit this for now to unblock things (the APIs are currently broken, returning null triggers a bug) but will open an issue to figure this out.

Issue: #32142

halter73 · 2020-02-11T00:51:45Z

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


-            return null;


I think we should continue returning null here too.

halter73 · 2020-02-11T00:57:43Z

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

@@ -95,4 +95,8 @@ public class QuicStreamAbortedException : QuicException
        public QuicStreamAbortedException(string message, long errorCode) : base(message) { }
        public long ErrorCode { get; }
    }
+    public class QuicOperationAbortedException : QuicException


I see that QuicException derives directly from Exception. Would it be better to have it derive from IOException or possibly OperationCanceledException if it's always user-initiated?

I would lean towards IOException given how it's currently used. I see it's being triggered by stuff like "HandleEventShutdownInitiatedByPeer" which feels really IOExceptiony to me (if it needs to be an exception at all).

Yea that seems reasonable. Open up an issue for it?

I also don't know if QuicOperationAbortedException will be necessary now if we start returning null.

I looked at SocketException, and it doesn't derive from IOException, so I'm no longer convinced this is the way to go. Maybe it should derive from SystemException (which SocketException does indirectly), but I'm not sure what the guidance there is. Is this supposed to be the base class for all Exceptions in a System.* namespace?

maryamariyan · 2020-02-12T18:11:20Z

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.

karelz · 2020-02-12T22:37:28Z

@maryamariyan this will be an interesting case as the API is in new library that we intentionally don't ship yet. The API has to go through API iterations and then API review. Then it will be good time to document it once we have final shape.
How does that fit our API documentation flow?

scalablecory added the area-System.Net label Feb 10, 2020

scalablecory added this to the 5.0 milestone Feb 10, 2020

scalablecory requested review from jkotalik and a team February 10, 2020 21:15

scalablecory added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Feb 10, 2020

scalablecory force-pushed the quic-aborts branch from e2a4da7 to 3b93c25 Compare February 10, 2020 22:22

scalablecory removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Feb 10, 2020

jkotalik reviewed Feb 10, 2020

View reviewed changes

jkotalik approved these changes Feb 10, 2020

View reviewed changes

Fix ConditionalClass not existing on new test class.

b579dfd

scalablecory self-assigned this Feb 11, 2020

halter73 reviewed Feb 11, 2020

View reviewed changes

scalablecory merged commit 2a054e7 into dotnet:master Feb 11, 2020

maryamariyan added the new-api-needs-documentation label Feb 12, 2020

scalablecory deleted the quic-aborts branch July 29, 2020 16:06

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement QUIC abort exceptions, fix channel usage, and implement some tests. #32051

Implement QUIC abort exceptions, fix channel usage, and implement some tests. #32051

scalablecory commented Feb 10, 2020 •

edited

Loading

jkotalik commented Feb 10, 2020

scalablecory commented Feb 10, 2020

scalablecory commented Feb 10, 2020

jkotalik Feb 10, 2020

scalablecory Feb 11, 2020

jkotalik Feb 10, 2020

halter73 Feb 11, 2020

jkotalik Feb 11, 2020

scalablecory Feb 11, 2020

jkotalik Feb 11, 2020

scalablecory Feb 11, 2020

scalablecory Feb 11, 2020

scalablecory Feb 11, 2020

halter73 Feb 11, 2020

halter73 Feb 11, 2020 •

edited

Loading

scalablecory Feb 11, 2020

jkotalik Feb 11, 2020

halter73 Feb 12, 2020

maryamariyan commented Feb 12, 2020

karelz commented Feb 12, 2020

Implement QUIC abort exceptions, fix channel usage, and implement some tests. #32051

Implement QUIC abort exceptions, fix channel usage, and implement some tests. #32051

Conversation

scalablecory commented Feb 10, 2020 • edited Loading

jkotalik commented Feb 10, 2020

scalablecory commented Feb 10, 2020

scalablecory commented Feb 10, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Feb 11, 2020 • edited Loading

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

maryamariyan commented Feb 12, 2020

karelz commented Feb 12, 2020

scalablecory commented Feb 10, 2020 •

edited

Loading

halter73 Feb 11, 2020 •

edited

Loading