Pipelines to use UnsafeQueueUserWorkItem

[benaadams]

ExecutionContext is handed externally by the scheduler

Fixes #32924

/cc @pakrym @davidfowl @stephentoub

[benaadams]

Needs #33637 first

[davidfowl]

Fixes https://github.com/dotnet/corefx/issues/32924

[stephentoub]

Everywhere PipeScheduler.Schedule is used either already flows context another way or doesn't need to?

And Schedule is public. We're confident no one is using it on their own, resulting in context no longer flowing when it should?

[benaadams]

Everywhere PipeScheduler.Schedule is used either already flows context another way or doesn't need to?

Yes it pre-captures; then does a dance with sync and exec context based on the captures; with different wrapping actions; so the issue is currently it double runs with the EC if it flows; once in the callback and once from the threadpool

corefx/src/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs

Lines 681 to 707 in 2f8cd58

	if (completionData.SynchronizationContext == null)
	{
	// We don't have a SynchronizationContext so execute on the specified scheduler
	if (completionData.ExecutionContext == null)
	{
	// We can run directly, this should be the default fast path
	scheduler.Schedule(completionData.Completion, completionData.CompletionState);
	return;
	}
	// We also have to run on the specified execution context so run the scheduler and execute the
	// delegate on the execution context
	scheduler.Schedule(s_scheduleWithExecutionContextCallback, completionData);
	}
	else
	{
	if (completionData.ExecutionContext == null)
	{
	// We need to box the struct here since there's no generic overload for state
	completionData.SynchronizationContext.Post(s_syncContextExecuteWithoutExecutionContextCallback, completionData);
	}
	else
	{
	// We need to execute the callback with the execution context
	completionData.SynchronizationContext.Post(s_syncContextExecutionContextCallback, completionData);
	}
	}

And Schedule is public. We're confident no one is using it on their own, resulting in context no longer flowing when it should?

I can't answer that

[benaadams]

Though... not sure how no context is the fast path. Looking into that

[benaadams]

Though... not sure how no context is the fast path.

Is coming from

public partial readonly struct ValueTaskAwaiter<TResult>
{
    IStateMachineBoxAwareAwaiter.AwaitUnsafeOnCompleted(IAsyncStateMachineBox box)
    {
        // ...
        Unsafe.As<IValueTaskSource<TResult>>(obj).OnCompleted(
            ValueTaskAwaiter.s_invokeAsyncStateMachineBox, 
            (object) box, 
            this._value._token, 
            ValueTaskSourceOnCompletedFlags.UseSchedulingContext);
        // ...
    }
}

So I'd assume correct

[benaadams]

And Schedule is public. ...

It probably should have copied TaskScheduler's approach e.g.

public abstract class PipeScheduler
{
    public static PipeScheduler ThreadPool { get; }
    public static PipeScheduler Inline { get; }

    protected internal abstract void Schedule(Action<object> action, object state);
}

[benaadams]

Better?

[davidfowl]

I'm not a fan of the complexity. I think it's fine to break that behavior.

[stephentoub]

I'm not a fan of the complexity. I think it's fine to break that behavior.

I understand and appreciate that viewpoint. I'm a little concerned though that we're breaking with expectation, since every other core library that "schedules" stuff flows EC, unless the method is prefixed with "Unsafe". Changing PipeScheduler.Schedule to not flow makes that harder to communicate, as it introduces ambiguity to that explanation.

If no one other than our own internal code uses and will ever use PipeScheduler.Schedule directly, then it's probably not a big deal. I agree it should have been protected internal instead of public.

[davidfowl]

@dotnet-bot test this please

[benaadams]

Will still fail because #33637 isn't merged

[davidfowl]

Ah

[davidfowl]

Maybe cleaner to add an UnsafeSchedule that's public?

[stephentoub]

Maybe cleaner to add an UnsafeSchedule that's public?

Sounds good to me. We'd want to run it through API review, but I wouldn't expect any pushback as it's following the pattern.

[jkotas]

@MichalStrehovsky We did not get ProjectN bits update in 12 days. Should we wait for the update, or should folks rather start adding exclusions to get the PRs through?

[MichalStrehovsky]

@MichalStrehovsky We did not get ProjectN bits update in 12 days. Should we wait for the update, or should folks rather start adding exclusions to get the PRs through?

I noticed that yesterday too and started an email thread with @zamont whether we know what's wrong, but it probably got buried in Zach's emails. Cc @nattress @sergiy-k

If this is blocking subsequent work and we can get unblocked by adding lines to ApiCompatBaseline.uapaot.txt, let's just do that. I'll do a sweep removing the lines and unblocking tests later.

[benaadams]

If this is blocking subsequent work and we can get unblocked by adding lines to ApiCompatBaseline.uapaot.txt, let's just do that. I'll do a sweep removing the lines and unblocking tests later.

Would need to be in this PR #33637 as that exposes the api that this PR uses

[stephentoub]

@dotnet-bot test this please