Handle concurrent reads and concurrent writes on MsQuicStream #67329

rzikm · 2022-03-30T10:38:45Z

This PR consolidates detection of concurrent reads and concurrent writes on MsQuicStream and throws InvalidOperationException with proper message.

Fixes #52627.

Analysis - Reads (ReadAsync): No changes were needed

When there is an outstanding read (i.e. a Task asynchronously waiting for incoming data), then _state.ReadState is set to ReadState.PendingRead. Any concurrent read will then read the state (under a lock), and then throw outside of the lock (without doing any changes to the stream state).

Concurrent reads which finish synchronously (or synchronous read followed by asynchronous read) are serialized correctly and don't lead to an exception (I suppose we are fine with this, as it does not corrupt the inner state).

Analysis - Writes (WriteAsync overloads)

All reads need to go through HandleWriteStartState, which set _state.SendState to SendState.Pending or SendState.Finished (if it was 0 size write). We need to check just before the assignment (still inside the lock) and if the _state.SendState is SendState.Pending or SendState.Finished, we clashed with a previous operation (either synchronous or asynchronous one).

ghost · 2022-03-30T10:38:54Z

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

Issue Details

This PR consolidates detection of concurrent reads and concurrent writes on MsQuicStream and throws InvalidOperationException with proper message.

Fixes #52627.

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

…nt-rw

CarnaViire · 2022-04-04T20:42:43Z

Reads (ReadAsync): No changes were needed

We've discussed in #67037 that after that change we cannot rely on state anymore, because there are things being done outside the lock, so the Read operations can start to overlap now... it's true that at the moment it works because the second operation doesn't touch things that the first one might need, but it is fragile, because something new might be added in the future (used by both operations), break that unspoken contract and blow up unexpectedly, at a moment that will be super hard to catch...

rzikm · 2022-04-06T07:41:44Z

We've discussed in #67037 that after that change we cannot rely on state anymore, because there are things being done outside the lock, so the Read operations can start to overlap now...

I am not sure I follow, the operations are still being serialized by the lock (_state) statement, in #67037, we just deferred some calls to MsQuic until the lock is released, which I think does not affect the ReadState.

I originally wanted to add explicit guards (like it is done in SslStream) but after a conversation with @ManickaP, I was convinced that it would be better to not add any additional state variables if we can detect concurrent operations using the Read/Write state alone.

ManickaP · 2022-04-06T21:26:13Z

One thing to add is that you need also take into account that you might get RECEIVE_EVENT from msquic in parallel, which might change the ReadState. And I think we do have a race here (and we had it even before moving EnableReceive out of lock):

caller 1: ReadAsync is called, we return ValueTask and set PendingRead, caller 1 is awaiting
msquic thread: RECEIVE_EVENT comes, gets a lock, changes the state to None and copies the buffer to the user buffer, releases lock, and thread is switched (we haven't returned result to msquic yet, nor completed the ValueTask)
caller 2: ReadAsync is called, takes lock, state is None and is changed to PendingRead, returns ValueTask of the same resetable source as caller 1 has
msquic thread: back to RECEIVE_EVENT, the resettable source gets completed, we return Success/Continue to msquic

Please correct me here if I'm missing some clever trick that prevents what I've just described from happening.

Anyway, I have no idea what happens when we return twice the same resettable completion source. And if this doesn't blow up, what happens when both waiters gets signaled.

ManickaP · 2022-04-06T21:26:13Z

One thing to add is that you need also take into account that you might get RECEIVE_EVENT from msquic in parallel, which might change the ReadState. And I think we do have a race here (and we had it even before moving EnableReceive out of lock):

caller 1: ReadAsync is called, we return ValueTask and set PendingRead, caller 1 is awaiting
msquic thread: RECEIVE_EVENT comes, gets a lock, changes the state to None and copies the buffer to the user buffer, releases lock, and thread is switched (we haven't returned result to msquic yet, nor completed the ValueTask)
caller 2: ReadAsync is called, takes lock, state is None and is changed to PendingRead, returns ValueTask of the same resetable source as caller 1 has
msquic thread: back to RECEIVE_EVENT, the resettable source gets completed, we return Success/Continue to msquic

Please correct me here if I'm missing some clever trick that prevents what I've just described from happening.

Anyway, I have no idea what happens when we return twice the same resettable completion source. And if this doesn't blow up, what happens when both waiters gets signaled.

rzikm · 2022-04-07T13:25:40Z

Please correct me here if I'm missing some clever trick that prevents what I've just described from happening.

No, this is actually legit and can conceivably happen, good catch! This instance seems to have an easy fix: just move state.ReceiveResettableCompletionSource.Complete(readLength); inside the lock in HandleEventRecv. So that the state of both the completion source and associated (Read|Write)State property on the stream State is updated "atomically", since these two need to be in sync. Thoughts, @ManickaP?

I see that we usually explicitly complete the completion source outside of the lock (I guess to minimize the critical section), so similar races may be elsewhere, I will try to look into it.

rzikm · 2022-04-07T14:50:09Z

So it seems that the write path does not suffer from the same problem because the SendState is reset on the reading thread only after the completion source is awaited and the cancellation/abortive paths transition to a final state so there shouldn't be similar races there.

rzikm · 2022-04-07T15:17:59Z

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

            }
        }

-        [Fact]


This test is redundant, the stream conformance tests already test this.

rzikm · 2022-04-08T12:38:37Z

/azp run runtime-libraries stress-http

azure-pipelines · 2022-04-08T12:38:47Z

Azure Pipelines successfully started running 1 pipeline(s).

rzikm · 2022-04-08T12:38:52Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2022-04-08T12:39:05Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP

If making sure there are no concurrent reads/writes is proving to be impossible to achieve with just the existing Read/SendState. Should we revisit the option to introduce an extra flag? We could pack it within the Read/SendState to save on space, but that might generate quite unreadable code.

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

ManickaP · 2022-04-08T14:18:30Z

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

-                state.ReceiveResettableCompletionSource.Complete(readLength);
+                // We're completing a pending read. This statement must be still inside a lock, otherwise another thread
+                // could see ReadState.None and access the still incomplete completion source
+                if (shouldComplete)


I was trying to figure out why we complete all the completion sources outside of the lock and could only find this historical comment: d028d30#r353958420

Can we move the task completion to outside of the lock?

So I cannot tell if moving this inside lock will cause any deadlocks (we use CompleteAsynchronously so it shouldn't) or have some other negative impact.

Also, if we need to move this one inside the lock, are there any other places we should investigate as well?

Lastly, I'd love @CarnaViire to give this look as well, she's the one with the most knowledge about reads.

I think we don't need to touch other completions, since they all happen on terminal SendStates, or -- in case of write's -- they are protected by intermediate state.

From offline discussion: let's try to keep the completion outside of the lock and introduce extra ReadState to imitate what is happening on the Write path

In the end I think we can do without adding additional ReadState, we just keep the PendingRead state a little longer.

I also wanted to avoid making ReadAsync into a state machine, so I reset the ReadState still on the callback thread by taking the lock again after completing the completion source.

scratch that, additional state was needed in the end, so I added PendingReadFinished, And I also had to make ReadAsync into state machine.

...libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamConnectedStreamConformanceTests.cs

…nt-rw

CarnaViire · 2022-04-26T12:47:48Z

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

                switch (state.ReadState)
                {
                    case ReadState.None:
+                    case ReadState.PendingReadFinished:


I assume there's a race that makes it possible to enter the callback and the lock it this state?

Yes, the callback simply fires twice in a row before the other thread has time to reset the ReadState. Do you think this needs a comment in the code?

Yes, especially with current comment only explaining "None" state 😄

Are you sure the reading flow is fine in that case?

And - just on a side note - oh god I have a bad feeling about that 😢 the states becoming a bit blurred and still not fully helping us serialize stuff in a way we want to. ☹️

CarnaViire · 2022-04-26T18:55:03Z

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

            {
                case ReadState.PendingRead:
-                    ex = new InvalidOperationException("Only one read is supported at a time.");
+                case ReadState.PendingReadFinished:


We also need to change one thing above (closer to the start of ReadAsync):

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

Line 451 in 42044c6

if (initialReadState != ReadState.PendingRead && cancellationToken.IsCancellationRequested)

it now should be
if (initialReadState != ReadState.PendingRead && initialReadState != ReadState.PendingReadFinished && cancellationToken.IsCancellationRequested)
because they both get this special treatment here

Good catch!

CarnaViire

LGTM, thanks!

rzikm added 2 commits March 30, 2022 11:44

Throw on concurrent operations

3919515

Remove redundant test

1bc1e90

ghost added the area-System.Net.Quic label Mar 30, 2022

ghost assigned rzikm Mar 30, 2022

rzikm changed the title ~~52627-quic-concurrent-rw~~ Handle concurrent reads and concurrent writes on MsQuicStream Mar 30, 2022

rzikm added 2 commits March 30, 2022 13:51

Move _state.SendState access under a lock

ad6cff8

Merge remote-tracking branch 'upstream/main' into 52627-quic-concurre…

700e797

…nt-rw

rzikm marked this pull request as ready for review March 30, 2022 12:15

rzikm requested review from CarnaViire and ManickaP April 1, 2022 06:41

Merge branch 'main' into 52627-quic-concurrent-rw

042df54

runfoapp bot mentioned this pull request Apr 6, 2022

TLS1.3 networking tests are flaky on Windows 11 #58927

Closed

Merge remote-tracking branch 'upstream/main'

2f3c8ca

rzikm commented Apr 7, 2022

View reviewed changes

Fix data race on ReceiveResettableCompletionSource

d59de1d

ManickaP reviewed Apr 8, 2022

View reviewed changes

runfoapp bot mentioned this pull request Apr 8, 2022

System.Net.Security.Tests CertificateValidationRemoteServer_EndToEnd_Ok failing on Windows 7 #65168

Closed

rzikm and others added 2 commits April 11, 2022 10:47

Remove unnecessary override

b1eb328

Merge branch 'main' into 52627-quic-concurrent-rw

b501b94

Move task completion outside of the lock

de30664

rzikm requested a review from ManickaP April 22, 2022 13:16

rzikm added 2 commits April 25, 2022 10:39

Fix failing tests

426ecdb

Merge remote-tracking branch 'upstream/main' into 52627-quic-concurre…

b150ec3

…nt-rw

CarnaViire reviewed Apr 26, 2022

View reviewed changes

rzikm added 2 commits April 26, 2022 20:21

Update comments

7454434

Add missing case

895978d

CarnaViire reviewed Apr 26, 2022

View reviewed changes

fixup! Add missing case

9d25082

CarnaViire approved these changes Apr 26, 2022

View reviewed changes

rzikm merged commit a077f27 into dotnet:main Apr 27, 2022

ghost locked as resolved and limited conversation to collaborators May 27, 2022

karelz added this to the 7.0.0 milestone Jul 19, 2022

                           }
                       }
-                      [Fact]

Handle concurrent reads and concurrent writes on MsQuicStream #67329

Handle concurrent reads and concurrent writes on MsQuicStream #67329

Uh oh!

Conversation

rzikm commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Analysis - Reads (ReadAsync): No changes were needed

Analysis - Writes (WriteAsync overloads)

Uh oh!

ghost commented Mar 30, 2022

Uh oh!

CarnaViire commented Apr 4, 2022

Uh oh!

rzikm commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManickaP commented Apr 6, 2022

Uh oh!

ManickaP commented Apr 6, 2022

Uh oh!

rzikm commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzikm commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzikm commented Apr 8, 2022

Uh oh!

azure-pipelines bot commented Apr 8, 2022

Uh oh!

rzikm commented Apr 8, 2022

Uh oh!

azure-pipelines bot commented Apr 8, 2022

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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 Mar 30, 2022 •

edited

Loading

rzikm commented Apr 6, 2022 •

edited

Loading

rzikm commented Apr 7, 2022 •

edited

Loading

rzikm commented Apr 7, 2022 •

edited

Loading