[QUIC] API QuicStream

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.

[ManickaP]

Fixes#69675

This PR is based on #71783 so it shows more changes than it actually contains, see https://github.com/ManickaP/runtime/compare/mapichov/quic-connection...ManickaP:runtime:mapichov/quic-stream?expand=1 for the actual diff.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

[ghost]

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]

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

Issue Details

---

Fixes #69675

This PR is based on #71783 so it shows more changes than it actually contains.

ATM depends on: microsoft/msquic#2883 but AFAIK only one outerloop test is failing because of this (the one testing stream shutdown due to idle connection). Depending on severity of this and speed on that msquic PR, I may temporarily re-introduce connection state from:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs

Lines 120 to 125 in b480845

	// TODO: remove once/if https://github.com/microsoft/msquic/pull/2872 is merged
	internalsealedclassState
	{
	publiclongAbortErrorCode=-1;
	}
	privateState_state=newState();

Also I'll be filling in comments, not everything is covered.

Kept in draft until API is approved.

Author:	ManickaP
Assignees:	-
Labels:	new-api-needs-documentation, area-System.Net.Quic
Milestone:	-

[CarnaViire]

Why is this inside do...while ? One iteration can copy, and the following can throw?

[ManickaP]

The loop can iterate at most twice. First, it tries to copy from the buffer, if it succeeds (data are already there) it will break out of the loop in while (... totalCopied == 0);. If no data are available in the buffer, the await valueTask.ConfigureAwait(false); will wait on the next RECEIVE event and once it arrives it will repeat the loop with copying, copy something (or learn about FIN flag) and break out of the loop.

The valueTask must be awaited each time it's retrieved, even if it's completed and it seems pointless - e.g.: _receiveTcs.TrySetResult();, since it's backed up by MRVTS which keeps "version" and the await will reset it (increment it).

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[ManickaP]

Done.

[CarnaViire]

I still don't understand how we would overcome the race when - between the first and the second iteration - a parallel read could come and "steal" the arrived data? What would happen to the first read? It seems like it could either throw or - would it start waiting on data to arrive again?

[CarnaViire]

How do we make sure send buffers are not disposed in the middle of send operation?

[ManickaP]

By awaiting the valueTask above which waits on SHUTDOWN_COMPLETE which is the last event on the stream and means both side are closed, so no send operation is in progress.

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

Added comment 3 lines above, at the valueTask await.

[wfurt]

We no longer do the ShutdownCompleted because we expect that in Dispose? Is there reason to delay that if we know this is final?

[ManickaP]

Delay what exactly? DisposeAsync is controlled from the calling code. In most cases the tests are just doing this:

awaitusingHttp3LoopbackStreamstream=awaitconnection.AcceptRequestStreamAsync();awaitstream.HandleRequestAsync();

And in some cases, the tests are playing with postponing the disposal to later, doing cancellations etc.

Potentially we could replace it with await Task.WhenAll(_stream.WritesClosed, _stream.ReadsClosed); which should correspond to SHUTDOWN_COMPLETE give or take. But I haven't experience any issues with the state as-is in this PR, so I don't particularly see reason to change it. After all, this (meaning as-is now) will be usage pattern done by the lib consumers.

[wfurt]

Generally looks ok to me. I wish we have better names for some of the handles but that is ok.
It seems like we also break the build/tests in few places and that should be fixed before merging.

[rzikm]

Looks good modulo existing comments

[rzikm]

Do we store the error code anywhere? it might be useful during dump investigation.

[ManickaP]

It'll stay set in the Exception in the _acceptQueue, so it's available, although less conveniently. Do you want me to reintroduce it? Is it something you used while doing investigations?

[rzikm]

A comment like "this will loop at most twice" would be nice :)

[rzikm]

Can you add a comment for this please? This is something I am going to forget very soon :D

[ManickaP]

All comments should be answered, only waiting on CI.

[CarnaViire]

Do I understand correctly, that complete can only become true in "synchronous" case? Meaning, we didn't actually wait for anything in valueTask above

[ManickaP]

Yes, but I'd formulate it differently: if complete was reported as true then the valueTask will complete synchronously. Note that the valueTask might have been completed (whether temporarily or finally) before it got to _receiveTcs.TrySetResult(final: true);.

[ManickaP]

One failure unrelated: #71233
One failure related (windows), reproduced locally, fixed and tested locally in a tight loop.
--> Merging now.

[ManickaP]

@JamesNK this will need your reaction as well. This contains the stream changes, most interesting will be the ReadsClosed, WritesClosed tasks.