Fix race condition with cancellation tokens in Read/Flush operations

[davidfowl]

• There's a tight race where UnsafeRegister can fire inline and then ReadAsync never completes. This is because the cancellation token property has not been assigned yet so the callback noops. This change checks the result of the UnsafeRegister operation to see if ran synchronously and throws if it did. We also move any state transitions to after these checks to make sure the PipeAwaitable state doesn't change before throwing.

Fixes #58909

cc @zlatanov

PS: We conveniently added an overload that takes the cancellation token in .NET 6 but I didn't try to use it here.

[zlatanov]

@davidfowl wouldn't it be a problem that the completion data isn't being zeroed out? Also the Pipe seem to use the completion data to schedule the next read / write.

Here Cancel(out completionData) will not be called:

runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeAwaitable.cs

Lines 139 to 150 in 57bfe47

	public void CancellationTokenFired(out CompletionData completionData)
	{
	// We might be getting stale callbacks that we already unsubscribed from
	if (CancellationToken.IsCancellationRequested)
	{
	Cancel(out completionData);
	}
	else
	{
	completionData = default;
	}
	}

And here the next TrySchedule will have default CompletionData:

runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs

Lines 1114 to 1122 in 57bfe47

	private void ReaderCancellationRequested()
	{
	CompletionData completionData;
	lock (SyncObj)
	{
	_readerAwaitable.CancellationTokenFired(out completionData);
	}
	TrySchedule(ReaderScheduler, completionData);
	}

And it seems that TrySchedule with default completion data is a noop.

runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs

Lines 783 to 806 in 57bfe47

	private static void TrySchedule(PipeScheduler scheduler, in CompletionData completionData)
	{
	Action<object?> completion = completionData.Completion;
	// Nothing to do
	if (completion is null)
	{
	return;
	}
	// Ultimately, we need to call either
	// 1. The sync context with a delegate
	// 2. The scheduler with a delegate
	// That delegate and state will either be the action passed in directly
	// or it will be that specified delegate wrapped in ExecutionContext.Run
	if (completionData.SynchronizationContext is null && completionData.ExecutionContext is null)
	{
	// Common fast-path
	scheduler.UnsafeSchedule(completion, completionData.CompletionState);
	}
	else
	{
	ScheduleWithContext(scheduler, in completionData);
	}
	}

This might as well be the desired behavior, I'm just pointing it out :)

[davidfowl]

In the cases where the PipeAwaitable returns no CompletionData it's a noop schedule. That's by design. It could be optimized by making these methods return a bool but this is how it has always worked.

[zlatanov]

In the cases where the PipeAwaitable returns no CompletionData it's a noop schedule. That's by design. It could be optimized by making these methods return a bool but this is how it has always worked.

This actually is my point. In case of synchronous cancellation callback, the completion data will be defaulted (but not reset). If it was asynchronous, it could contain actual data.

[davidfowl]

This actually is my point. In case of synchronous cancellation callback, the completion data will be defaulted (but not reset). If it was asynchronous, it could contain actual data.

That's the case that's being handled by this PR. It will run the callback, result in a noop, then throw synchronously from ReadAsync/FlushAsync (aka BeginOperation)

[adityamandaleeka]

@davidfowl Will you be backporting this to 6 once it's in?

[davidfowl]

6, 5, 3 yes

[ericstj]

Runtime test failiures are known issue on Win11, disabled tests in #59135

Runtime-staging failure is build race condition fixed by #59181

[davidfowl]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1239603000

[davidfowl]

/backport to release/6.0