keep MsQuicConnection alive when streams are pending #52800

wfurt · 2021-05-15T03:50:01Z

This change has two parts:

When there are stream active we need to keep at least the native parts of MsQuick connection alive. This solves problem when dangling stream is disposed by finalizer as well as it covers cases when the connection is explicitly disposed before the stream(s).

This is somewhat similar to #52776 but different in details. Instead of exposing the internal state, it mimics what we do with connection and stream in H/2. Connection has stream counter and if there are pending stream we would defer cleanup of the native state. That would happen later when last stream goes.

Second part implements suggestion from @geoffkizer to flush the accept queue on connection disposal. When working on that I noticed that the stream may get lost if AcceptQueue.Writer.TryWrite fails (because for example it is already closed. I added logic to check and dispose the stream as well.

This change is focused on the relation between connection and stream and it does not touch live cycle of either one.
I will to the stream cleanup in follow up PR.

fixes #52048

ghost · 2021-05-15T03:50:09Z

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

Issue Details

This change has two parts:

When there are stream active we need to keep at least the native parts of MsQuick connection alive. This solves problem when dangling stream is disposed by finalizer as well as it covers cases when the connection is explicitly disposed before the stream(s).

This is somewhat similar to #52776 but different in details. Instead of exposing the internal state, it mimics what we do with connection and stream in H/2. Connection has stream counter and if there are pending stream we would defer cleanup of the native state. That would happen later when last stream goes.

Second part implements suggestion from @geoffkizer to flush the accept queue on connection disposal. When working on that I noticed that the stream may get lost if AcceptQueue.Writer.TryWrite fails (because for example it is already closed. I added logic to check and dispose the stream as well.

This change is focused on the relation between connection and stream and it does not touch live cycle of either one.
I will to the stream cleanup in follow up PR.

fixes #52048

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

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

ManickaP · 2021-05-17T08:44:14Z

This is gonna clash with #52704, just FYI.

wfurt · 2021-05-17T19:12:54Z

yes, I know @ManickaP. I started the crash investigation independently of what @CarnaViire was boring on.
I do think we will need some linkage between stream and connection for all this to work properly.
We can possibly play game with the SafeHandles and all the DangerousAddRef makes me worry

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

…_52048

wfurt · 2021-06-01T09:44:44Z

ping? Any brave soul for review?

ManagedAVE_MinimalFailingTest does not crash but it still sometimes fail with timeout. I think that is separate issue and I filed microsoft/msquic#1668 to investigate. It seems to be specific to calling close from finalizer thread.

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

CarnaViire

LGTM

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

jkotas · 2021-06-01T14:28:01Z

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

+                    {
+                        GCHandle gcHandle = GCHandle.FromIntPtr(_closingGCHandle);
+                        Debug.Assert(gcHandle.IsAllocated);
+                        if (gcHandle.IsAllocated) gcHandle.Free();


Suggested change

if (gcHandle.IsAllocated) gcHandle.Free();

gcHandle.Free();

is this generally safe? We have other places with identical check.

IsAllocated just checks whether the GCHandle is not IntPtr.Zero. _closingGCHandle != IntPtr.Zero above does that already, so it should not be needed here.

wfurt · 2021-06-03T12:30:08Z

The linked issue was resolved by moving Handle.Release() outside of lock block. It seems like finalizing the connection can flush some events and cause unexpected processing.

ManickaP

LGTM appart from one question.

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

ManickaP · 2021-06-03T12:57:29Z

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

@@ -43,6 +42,7 @@ internal sealed class MsQuicConnection : QuicConnectionProvider
        internal sealed class State
        {
            public SafeMsQuicConnectionHandle Handle = null!; // set inside of MsQuicConnection ctor.
+            public GCHandle StateGCHandle;


I don't fully understand why was the StateGCHandle moved inside State?

When the connection is gone and state is kept alive because of the streams, we still want to lock state in place because we handed pointer to MsQuic. When we release all the streams, the original location is not accessible as we don't have link back to connection (and it may be gone) I did the move (sort of) in previous round using IntPtr. @CarnaViire and @jkotas suggested to preserve the type so I end up with this.

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

scalablecory · 2021-06-08T06:08:45Z

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

+                _state.AcceptQueue.Writer.Complete();
+            } catch { };
+
+            await foreach (MsQuicStream item in _state.AcceptQueue.Reader.ReadAllAsync())


Suggested change

await foreach (MsQuicStream item in _state.AcceptQueue.Reader.ReadAllAsync())

await foreach (MsQuicStream item in _state.AcceptQueue.Reader.ReadAllAsync().ConfigureAwait(false))

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

scalablecory · 2021-06-08T06:16:40Z

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

+                }
+            }
+
+            FlushAcceptQueue().GetAwaiter().GetResult();


Connections are IAsyncDisposable; we should make dispose do proper await there.

I'm not sure what you suggest. We still need to flush for synchronous Dispose(). Can we do what ever needs to be done as follow-up? I would like to get this this in to get verifications on the crashes.

That's fine.

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

…_52048

wfurt · 2021-06-10T15:49:17Z

Socket failure on macOS is unrelated.

wfurt added 2 commits May 14, 2021 20:26

keep MsQuicConnection alive when streams are pending

b24b9d5

remove extra file

93dbb7a

wfurt added the area-System.Net.Quic label May 15, 2021

wfurt requested review from CarnaViire, scalablecory, jkotas, ManickaP and geoffkizer May 15, 2021 03:50

wfurt self-assigned this May 15, 2021

gfoidl reviewed May 15, 2021

View reviewed changes

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

CarnaViire mentioned this pull request May 17, 2021

[QUIC] Handle connection abort in streams #52776

Merged

CarnaViire reviewed May 17, 2021

View reviewed changes

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

scalablecory reviewed May 18, 2021

View reviewed changes

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

runfoapp bot mentioned this pull request May 20, 2021

InvokeCodeThatShouldFirEvents_EnsureEventsFired fails on OSX #52710

Closed

ManickaP mentioned this pull request May 26, 2021

QUIC stream limits #52704

Merged

Merge branch 'main' of https://github.com/dotnet/runtime into streams…

0012ed2

…_52048

runfoapp bot mentioned this pull request May 28, 2021

The type initializer for 'Wasm.Build.Tests.BuildTestBase' threw an exception. #53428

Closed

fix gchandle

5572326

CarnaViire reviewed Jun 1, 2021

View reviewed changes

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

CarnaViire reviewed Jun 1, 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 1, 2021

View reviewed changes

jkotas reviewed Jun 1, 2021

View reviewed changes

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

jkotas reviewed Jun 1, 2021

View reviewed changes

feedback from review

f268e01

ManickaP approved these changes Jun 3, 2021

View reviewed changes

scalablecory approved these changes Jun 8, 2021

View reviewed changes

wfurt mentioned this pull request Jun 9, 2021

[System.Net.Quic]Release configuration resource. #53857

Merged

wfurt added 4 commits June 9, 2021 14:15

Merge branch 'main' of https://github.com/dotnet/runtime into streams…

188cc2b

…_52048

feedback from review

cd40a5b

feedback from review

ade7ec5

Merge branch 'main' of https://github.com/dotnet/runtime into streams…

b161cf5

…_52048

wfurt merged commit 68484ea into dotnet:main Jun 10, 2021

wfurt deleted the streams_52048 branch June 10, 2021 15:49

wfurt mentioned this pull request Jun 15, 2021

fix MsQuicStream counting on failure #54249

Merged

CarnaViire mentioned this pull request Jun 18, 2021

[HTTP/3] Fix NullReferenceException on cancellation #54334

Closed

wfurt mentioned this pull request Jul 6, 2021

QuicConnection.Dispose should not wait for streams to complete #55156

Closed

ghost locked as resolved and limited conversation to collaborators Jul 10, 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

keep MsQuicConnection alive when streams are pending #52800

keep MsQuicConnection alive when streams are pending #52800

wfurt commented May 15, 2021

ghost commented May 15, 2021

ManickaP commented May 17, 2021

wfurt commented May 17, 2021

wfurt commented Jun 1, 2021

CarnaViire left a comment

jkotas Jun 1, 2021

wfurt Jun 1, 2021

jkotas Jun 1, 2021

wfurt commented Jun 3, 2021

ManickaP left a comment

ManickaP Jun 3, 2021

wfurt Jun 3, 2021

scalablecory Jun 8, 2021

scalablecory Jun 8, 2021

wfurt Jun 9, 2021

scalablecory Jun 9, 2021

wfurt commented Jun 10, 2021

	await foreach (MsQuicStream item in _state.AcceptQueue.Reader.ReadAllAsync())
	await foreach (MsQuicStream item in _state.AcceptQueue.Reader.ReadAllAsync().ConfigureAwait(false))

keep MsQuicConnection alive when streams are pending #52800

keep MsQuicConnection alive when streams are pending #52800

Conversation

wfurt commented May 15, 2021

ghost commented May 15, 2021

ManickaP commented May 17, 2021

wfurt commented May 17, 2021

wfurt commented Jun 1, 2021

CarnaViire left a 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

wfurt commented Jun 3, 2021

ManickaP left a 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

wfurt commented Jun 10, 2021