vs-threading icon indicating copy to clipboard operation
vs-threading copied to clipboard

JoinableTaskSynchronizationContext should override CreateCopy()

Open AArnott opened this issue 7 years ago • 2 comments

Bug description

The default implementation on SynchronizationContext.CreateCopy() returns a new instance of its own type. But that type has very different behavior than JoinableTaskSynchronizationContext, so it would be incorrect to return the base type from our derived type.

Repro steps

I don't know what failures this may lead to yet. But given when ExecutionContext is captured it can (sometimes?) flow SynchronizationContext, I suspect if we can isolate when that happens we can find a repro for a bug.

Expected behavior

We should override the CreateCopy() method and have it return a copy of the current instance. The copy should behave exactly like the original, including common values for all fields so that they share queues, etc. But it should be a unique instance (not this) to avoid exposing bugs in others' code such as that learned by WPF, as pointed out by @weltkante in #347.

Additional context

@jviau noticed some test failures while working on the ReentrantSemaphore in certain conditions that he attributed to our CreateCopy() behavior, IIRC.

AArnott avatar Aug 14 '18 16:08 AArnott

I can't recall it perfectly, but I think the issues I ran into was around asserting exactly what value the sync context was. I hit issues whenever I yielded and then checked the context. It would have changed to the base sync context type due to the CreateCopy.

jviau avatar Aug 14 '18 17:08 jviau

I believe the problem mentioned in the WPF comments references SynchronizationContextTaskScheduler TryExecuteTaskInline, though there may be more places which make this assumption which I don't know of.

The point is when ExecutionContext flows the SynchronizationContext it will call CreateCopy instead of sharing the SynchronizationContext. The TaskScheduler assumes that having the same reference to the SC still means being on the same thread.

One way to trigger this problem is calling Wait on a task which hasn't been scheduled yet. (Note that I never experienced this myself I'm just piecing it together from the comments in the reference source, trying to build a repro of the issue described there.)

class Program
{
    [STAThread]
    static void Main(string[] args)
    {
        EventHandler handler = null;
        Application.Idle += handler = delegate
        {
            Application.Idle -= handler;
            SynchronizationContext.SetSynchronizationContext(new BrokenSyncContext(
                (WindowsFormsSynchronizationContext)SynchronizationContext.Current));

            var originalContext = ExecutionContext.Capture();
            var originalThreadId = Environment.CurrentManagedThreadId;
            var scheduler = TaskScheduler.FromCurrentSynchronizationContext();

            // leave the UI thread while flowing the SynchronizationContext
            // (there are probably other ways this can happen?)
            //
            // note this will call CreateCopy on the SynchronizationContext
            // and not just share it by reference
            //
            var t = Task.Run(() =>
            {
                ExecutionContext.Run(originalContext, s =>
                {
                    Debug.Assert(SynchronizationContext.Current is BrokenSyncContext,
                        "expecting SynchronizationContext to flow with ExecutionContext");

                    Debug.Assert(Environment.CurrentManagedThreadId != originalThreadId,
                        "expecting Task.Run to execute outside the UI thread");

                    // start enough tasks so we have a chance to find an unscheduled task to Wait() on
                    var list = new List<Task>();
                    for (int i = 0; i < Environment.ProcessorCount * 2; i++)
                    {
                        list.Add(Task.Factory.StartNew(() =>
                        {
                            Debug.Assert(SynchronizationContext.Current is BrokenSyncContext,
                                "expecting the scheduler to run us on the right SC");

                            Debug.Assert(Environment.CurrentManagedThreadId == originalThreadId,
                                "expecting the scheduler to schedule us back to the original thread");

                            var time = DateTime.UtcNow;
                            while ((DateTime.UtcNow - time).TotalMilliseconds < 100)
                            {
                                // block the core to reduce chance of other tasks
                                // being scheduled before we get a chance to wait
                            }
                        }, CancellationToken.None, TaskCreationOptions.None, scheduler));
                    }

                    // iterate backwards to get a higher chance to hit an unscheduled task first
                    for (int i = list.Count - 1; i >= 0; i--)
                    {
                        // waiting on an unscheduled task will try to inline it
                        // SynchronizationContextTaskScheduler will inline by reference equality
                        // This assumes that when ExecutionContext flows the SynchronizationContext by
                        // calling CreateCopy an actual copy is returned, not the same instance reused
                        list[i].Wait();
                    }

                    Application.Exit();
                }, null);
            });
        };
        Application.Run();
    }
}

// didn't want to bother implementing a full SynchronizationContext so I'm letting
// WinForms do the hard work and just override CreateCopy to present the bug
class BrokenSyncContext : SynchronizationContext
{
    private WindowsFormsSynchronizationContext _sc;

    public BrokenSyncContext(WindowsFormsSynchronizationContext sc) { _sc = sc; }

    public override void Post(SendOrPostCallback d, object state) => _sc.Post(d, state);
    public override void Send(SendOrPostCallback d, object state) => _sc.Send(d, state);

    // -- this version will trigger asserts
    public override SynchronizationContext CreateCopy() => this;

    // -- this version is not broken
    //public override SynchronizationContext CreateCopy() => new BrokenSyncContext(_sc);
}

weltkante avatar Aug 14 '18 19:08 weltkante