[QUIC] Call silent shutdown in case CloseAsync failed.

[ManickaP]

1. Makes cancellation in ValueTaskSource temporary, which enables receiving the expected MsQuic signal even after cancellationToken fired. In this case SHUTDOWN_COMPLETE.
2. If ValueTaskSource is cancelled, remember what happened to it before the caller read the OCE so that it can be restored to the task afterwards.
3. When the OCE is delivered, restore ValueTaskSource to the remembered state (awaiting or completed with the result).
4. Lastly, examine the state of _shutdownTcs in Dispose and decide whether the silent shutdown needs to be called.

Changes are in ValueTaskSource and QuicConnection. Other changes are cosmetic, not functional (comments, code formatting).

Fixes #78641

I also did a manual testing with cancellation between CloseAsync --> connection.Shutdown and SHUTDOWN_COMPLETED by inserting delay to the event handler, but this as this is timing sensitive it's impossible to do reliably in a test.

[ghost]

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

Issue Details

---

1. Makes cancellation in ValueTaskSource temporary, which enables receiving the expected MsQuic signal even after cancellationToken fired. In this case SHUTDOWN_COMPLETE.
2. If ValueTaskSource is cancelled, remember what happened to it before the caller read the OCE so that it can be restored to the task afterwards.
3. When the OCE is delivered, restore ValueTaskSource to the remembered state (awaiting or completed with the result).
4. Lastly, examine the state of _shutdownTcs in Dispose and decide whether the silent shutdown needs to be called.

Changes are in ValueTaskSource and QuicConnection. Other changes are cosmetic, not functional (comments, code formatting).

Fixes #78641

I also did a manual testing with cancellation between CloseAsync --> connection.Shutdown and SHUTDOWN_COMPLETED by inserting delay to the event handler, but this as this is timing sensitive it's impossible to do reliably in a test.

Author:	ManickaP
Assignees:	-
Labels:	area-System.Net
Milestone:	-

[ghost]

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

Issue Details

---

1. Makes cancellation in ValueTaskSource temporary, which enables receiving the expected MsQuic signal even after cancellationToken fired. In this case SHUTDOWN_COMPLETE.
2. If ValueTaskSource is cancelled, remember what happened to it before the caller read the OCE so that it can be restored to the task afterwards.
3. When the OCE is delivered, restore ValueTaskSource to the remembered state (awaiting or completed with the result).
4. Lastly, examine the state of _shutdownTcs in Dispose and decide whether the silent shutdown needs to be called.

Changes are in ValueTaskSource and QuicConnection. Other changes are cosmetic, not functional (comments, code formatting).

Fixes #78641

I also did a manual testing with cancellation between CloseAsync --> connection.Shutdown and SHUTDOWN_COMPLETED by inserting delay to the event handler, but this as this is timing sensitive it's impossible to do reliably in a test.

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

[rzikm]

I can still get DisposeAsync to throw like this.

var (clientOptions, _, listenerOptions) = RuntimeSandbox.Helpers.GetDefaultOptions();

await using var pair = await RuntimeSandbox.Helpers.GetConnectedPair(clientOptions, listenerOptions);
var (client, server) = pair;

var cts = new CancellationTokenSource();
var task = client.CloseAsync(0).AsTask();

cts.Cancel(); // throws even without this line
await client.DisposeAsync();
// Unhandled exception. System.InvalidOperationException: Operation is not valid due to the current state of the object.
//    at System.Threading.Tasks.Sources.ManualResetValueTaskSourceCore`1.OnCompleted(Action`1 continuation, Object state, Int16 token, ValueTaskSourceOnCompletedFlags flags)
//    at System.Net.Quic.ValueTaskSource.System.Threading.Tasks.Sources.IValueTaskSource.OnCompleted(Action`1 continuation, Object state, Int16 token, ValueTaskSourceOnCompletedFlags flags)
//    at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AwaitUnsafeOnCompleted[TAwaiter](TAwaiter& awaiter, IAsyncStateMachineBox box)
// --- End of stack trace from previous location ---
//    at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
//    at System.Threading.QueueUserWorkItemCallbackDefaultContext.Execute()
//    at System.Threading.ThreadPoolWorkQueue.Dispatch()
//    at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
//    at System.Threading.Thread.StartCallback()

Do we care? If not then LGTM.

[CarnaViire]

@rzikm it seems like it's a different case, not related to this cancellation issue? (do I understand correctly that this "Operation is not valid due to the current state of the object" happens in main, and continues to happen after the PR)?

Ok, I've re-read the stack-trace, and now I don't know if it's related or not 😅 but I'm curious whether this was happening before.

But I think we should care 🤔 I don't think it's ok to throw if dispose and/or close are called in parallel.

[ManickaP]

I can still get DisposeAsync to throw like this.

Just FYI, this happens without this change as well. I'll try looking into it if it's something small that I could put into this PR and if not I'll file separate issue.

[rzikm]

But I think we should care 🤔 I don't think it's ok to throw if dispose and/or close are called in parallel.

I think we should never throw from Dispose(Async)

[ManickaP]

Radek's problem fixed and added test for that.
I have reverted changes in ValueTaskSource and replaced the _shutdownTcs with TaskCompletionSource as Radek's use-case uncovered that we await the same task from 2 points (CloseAsync and DisposeAsync) which is OK if done serially but not in parallel. The easiest way out was to swap it for ordinary Task.

[CarnaViire]

You are creating an instance here every time regardless whether the exchange happened or not... is that expected?

[ManickaP]

I did not bother with optimizing it. It'll be at most 2 instances (in Gen0) in vain per connection lifetime. I can change it to use another field as a sentinel (e.g. int shutdownCalled) or to preallocate this in other field for a swap. Or just not guard against multiple Shutdown calls 🤷

[CarnaViire]

LGTM

[ManickaP]

Just FYI, using ordinary TCS brought back an issue with GC eating non-rooted QuicConnection while waiting on SHUTDOWN_COMPLETE, so I'll be re-thinking this once again. I'll ping you for re-review when the fix is up.

[ManickaP]

Failure is unrelated and known: #97546