Swallow ObjectDisposedException when aborting QuicStream from CancellationAction

[rzikm]

Fixes #73688.

Depends on #74669 (it changes the type of exception thrown).

When QuicStream.(Read|Write)Async is canceled via CancellationTokenSource.CancelAfter, the CancellationAction is invoked from different thread and therefore the Abort call races with Dispose on the original thread. #74669 will make the StreamClose/StreamAbort race safe by making sure we AddRef/Release the related MsQuicSafeHandle, thus converting the race to possible ObjectDisposedException which we can swallow.

[ghost]

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

Issue Details

---

Fixes #73688.

Depends on #74611 (it changes the type of exception).

When QuicStream.(Read|Write)Async is canceled via CancellationTokenSource.CancelAfter, the CancellationAction is invoked from different thread and therefore the Abort call races with Dispose on the original thread. #74611 will make the StreamClose/StreamAbort race safe by making sure we AddRef/Release the related MsQuicSafeHandle, thus converting the race to possible ObjectDisposedException which we can swallow.

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

[CarnaViire]

Should we add the test from #73688 (comment)?

[wfurt]

I'm wondering if we can have private version of stream.Abort that does not throw and either is silent or returns bool for tracing if we ant to know if Abort succeeded or not.

[CarnaViire]

Abort already tries to be no-op in case stream is disposed. But it is not enough. The only way to fix that I can think of, is to do dangerousAddRef in the beginning of Abort, and only after that check _disposed. Or wrap both msquic abort call and dispose of the handle in the lock and check _disposed inside the lock before abort. Otherwise, by the time we access handle on msquic abort call, it might get disposed already, despite the check in the beginning of Abort.

[wfurt]

So the ODE would come from the SafeHandle? I'm fine with this change for 7.0. It seems to cause lot of CI troubles. We can figure later if there is cheaper way to do it.

[rzikm]

The ObjectDisposedException is coming from DangerousAddRef. At the moment I see no way of avoiding the exception without a lock (we don't have TryAddRef or similar). I expect the exception to be relatively rare so I think it is fine (at least for now)

[ManickaP]

LGTM. Thanks for hunting this down!

[rzikm]

Should we add the test from #73688 (comment)?

I don't think I can make the test reliable enough to be worth it, the repro in the comment required a Console.ReadLine in QuicStream.Abort in order to work. I think the existing test we were hitting this on (StreamConformanceTests.ReadAsync_CancelPendingValueTask_ThrowsCancellationException) is sufficient (I used it as a model for my repro test). At its core, it uses

        protected async Task ValidateCancelableReadAsyncValueTask_AfterInvocation_ThrowsCancellationException(Stream stream, int cancellationDelay)
        {
            if (!stream.CanRead || !FullyCancelableOperations)
            {
                return;
            }

            var cts = new CancellationTokenSource();

            Task<int> t = stream.ReadAsync(new byte[1], cts.Token).AsTask();

            cts.CancelAfter(cancellationDelay);
            await AssertCanceledAsync(cts.Token, () => t);
        }

And once this finishes, it immediately starts disposing the streams/connections. (via using StreamPair streams).

[wfurt]

While it would be nice to have dedicated test I think it is ok if we don't - since the issue showed up in existing tests.

[rzikm]

Close+Open to trigger CI

[rzikm]

CI Failure is #74795

[rzikm]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3005534353