76831-SystemNetQuicTestsQuicStreamTestsWriteCanceled_NextWriteThrows

[rzikm]

Fixes #76831.

There was a data race between cancellation action firing (i.e. calling Abort()) and DisposeAsync() from the thread awaiting the cancelled Task.

Consider following code

                {
                    await using QuicStream stream = await connection.AcceptInboundStreamAsync();
                    CancellationTokenSource cts = new CancellationTokenSource(timeout);
                    //....
                    await stream.WriteAsync(buffer, cts.Token);
                }

If the cts times out, then the last await throws, which makes execution leave the block and triggering the DisposeAsync on stream. This dispose does graceful stream close. Which is racing with Abort() being called from the timer thread which handles the cts timeout.

This PR fixes the above situation by holding a lock while servicing the cancellation action (Abort), which mutually excludes DisposeAsync from gracefully closing the stream at the same time.

[ghost]

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

Issue Details

---

Fixes #76831.

There was a data race between cancellation action firing (i.e. calling Abort()) and DisposeAsync() from the thread awaiting the cancelled Task.

Consider following code

                {
                    await using QuicStream stream = await connection.AcceptInboundStreamAsync();
                    CancellationTokenSource cts = new CancellationTokenSource(timeout);
                    //....
                    await stream.WriteAsync(buffer, cts.Token);
                }

If the cts times out, then the last await throws, which makes execution leave the block and triggering the DisposeAsync on stream. This dispose does graceful stream close. Which is racing with Abort() being called from the timer thread which handles the cts timeout.

This PR fixes the above situation by holding a lock while servicing the cancellation action (Abort), which mutually excludes DisposeAsync from gracefully closing the stream at the same time.

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

[ManickaP]

I'm not sure this is still relevant after my last PR (#90253) in QuicStream, which reverted the order or calling abort and waking up the caller of the async operation:

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

Lines 72 to 88 in 963954a

	CancellationAction = target =>
	{
	try
	{
	if (target is QuicStream stream)
	{
	stream.Abort(QuicAbortDirection.Read, stream._defaultErrorCode);
	stream._receiveTcs.TrySetResult();
	}
	}
	catch (ObjectDisposedException)
	{
	// We collided with a Dispose in another thread. This can happen
	// when using CancellationTokenSource.CancelAfter.
	// Ignore the exception
	}
	}

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

Lines 93 to 111 in 963954a

	private readonly ResettableValueTaskSource _sendTcs = new ResettableValueTaskSource()
	{
	CancellationAction = target =>
	{
	try
	{
	if (target is QuicStream stream)
	{
	stream.Abort(QuicAbortDirection.Write, stream._defaultErrorCode);
	}
	}
	catch (ObjectDisposedException)
	{
	// We collided with a Dispose in another thread. This can happen
	// when using CancellationTokenSource.CancelAfter.
	// Ignore the exception
	}
	}
	};

For write, we now also wait for SEND_COMPLETE in all cases, including cancellation.

[rzikm]

Closing, the bug was fixed by #90253