[QUIC] MsQuicStream ctor code shuffles

[ManickaP]

We should move this before callback handler registration.

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

Lines 120 to 124 in f59a132

	if (!connectionState.TryAddStream(this))
	{
	_state.StateGCHandle.Free();
	throw new ObjectDisposedException(nameof(QuicConnection));
	}

We should set it before SetCallbackHandlerDelegate since we can get CONNECTION_CLOSED event and that needs this.
If we want to prevent decrementing stream count, we should explicitely unset here.

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

Line 126 in f59a132

_state.ConnectionState = connectionState;

The same applies to outbound ctor.

[ghost]

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

Issue Details

---

We should move this before callback handler registration.

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

Lines 120 to 124 in f59a132

	if (!connectionState.TryAddStream(this))
	{
	_state.StateGCHandle.Free();
	throw new ObjectDisposedException(nameof(QuicConnection));
	}

We should set it before SetCallbackHandlerDelegate since we can get CONNECTION_CLOSED event and that needs this.
If we want to prevent decrementing stream count, we should explicitely unset here.

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

Line 126 in f59a132

_state.ConnectionState = connectionState;

Author:	ManickaP
Assignees:	-
Labels:	area-System.Net.Quic, untriaged
Milestone:	-

[karelz]

Failure:

Process terminated. Assertion failed.
Exception occurred during handling Stream SHUTDOWN_COMPLETE event: System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.HandleEventConnectionClose(State state) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs:line 1368
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.HandleEventShutdownComplete(State state, StreamEvent& evt) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs:line 1024
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.HandleEvent(State state, StreamEvent& evt) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs:line 860
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.HandleEvent(State state, StreamEvent& evt) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs:line 860
   at System.Net.Quic.Implementations.MsQuic.MsQuicStream.NativeCallbackHandler(IntPtr stream, IntPtr context, StreamEvent& streamEvent) in /_/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs:line 824

Observed in PR #57598 - System.Net.Http.Functional.Tests on Fedora.34

[karelz]

It is unclear how often it shows up in CI -- we can't easily query Kusto for this :( ... bumping it to 6.0.
Per @CarnaViire it is simple fix. Given the (bad) impact on CI, we should try to get it in IMO ...

[karelz]

Test failures 7/30-8/23 (incl. PRs):

Day	Run	Test suite
8/17	PR #57530	System.Net.Http.Functional.Tests
8/17	PR #57533	System.Net.Http.Functional.Tests
8/17	PR #57546	System.Net.Http.Functional.Tests
8/17	PR #57078	System.Net.Http.Functional.Tests
8/17	PR #57550	System.Net.Http.Functional.Tests
8/17	PR #57536	System.Net.Http.Functional.Tests
8/17	PR #57581	System.Net.Http.Functional.Tests
8/17	PR #57303	System.Net.Http.Functional.Tests
8/17	PR #57457	System.Net.Http.Functional.Tests
8/17	PR #57598	System.Net.Http.Functional.Tests
8/17	PR #56501	System.Net.Http.Functional.Tests
8/18	PR #51736	System.Net.Http.Functional.Tests
8/18	PR #57303	System.Net.Http.Functional.Tests
8/19	Fixed in main via PR #57655
8/19	PR #57700 - release/6.0	System.Net.Http.Functional.Tests
8/19	PR #57762 - release/6.0-rc1	System.Net.Http.Functional.Tests
8/20	PR #57765 - release/6.0	System.Net.Http.Functional.Tests
8/20	PR #57740 - release/6.0	System.Net.Http.Functional.Tests
8/20	PR #57834 - release/6.0	System.Net.Http.Functional.Tests
8/20	PR #57851 - release/6.0-rc1	System.Net.Http.Functional.Tests
8/23	Fixed in release/6.0 via PR #57742
8/25	2x PR #58059 - release/6.0-rc1
8/26	PR #58148 - release/6.0-rc1
8/27	PR #58271 - release/6.0-rc1
8/28	PR #58316 - release/6.0-rc1

[karelz]

Given the frequency in CI, we should backport it to 6.0. Reopening to track that.
@CarnaViire can you please kick off the backport? Thanks!

[ViktorHofer]

Yes, this is happening in release/6.0 as well: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-57765-merge-d32bb18c1a0f47aba1/System.Net.Http.Functional.Tests/1/console.a56010ed.log?sv=2019-07-07&se=2021-09-09T00%3A07%3A38Z&sr=c&sp=rl&sig=bGIxW9Kk0xYnJ6I5POlZmWCK1EPNV1csEQDBxro7LHY%3D

Given the frequency in CI, we should backport it to 6.0

Shouldn't we also backport into the rc1 branch?

[karelz]

@ViktorHofer

Shouldn't we also backport into the rc1 branch?

Depends on appetite for RC1.
I assumed we are primarily trying to stabilize it. MsQuic is Preview only in 6.0 and the RC1 branch won't be used that much / long.
I let @danmoseley decide if he wants it to RC1 as well when he gets back on Monday.

[ViktorHofer]

As long as the fix lands in any branch that will exist for the next few years I'm good. Was mostly curious about why RC1 vs RC2, stabilization of RC1 makes sense.

[karelz]

Fixed for 6.0 RC2 in release/6.0 branch in PR #57742
Fixed for 7.0 in PR #57655