3

This is not a duplicate of Task not garbage collected. The symptoms are similar though.

The code below is a console app that creates an STA thread for use with WinForms. Tasks are posted to that thread via a custom task scheduler obtained with TaskScheduler.FromCurrentSynchronizationContext, which just implicitly wraps an instance of WindowsFormsSynchronizationContext here.

Depending on what causes this STA thread to end, the final task var terminatorTask = Run(() => Application.ExitThread()), scheduled in the WinformsApartment.Dispose method, may not always be getting a chance to execute. Regardless of that, I believe this task still should be getting garbage-collected, but it isn't. Why?

Here's a self-contained example illustrating that (s_debugTaskRef.IsAlive is true at the finish), tested with .NET 4.8, both Debug and Release:

using System;
using System.Diagnostics;
using System.Threading;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace ConsoleTest
{
    class Program
    {
        // entry point
        static async Task Main(string[] args)
        {
            try
            {
                using (var apartment = new WinformsApartment(() => new Form()))
                {
                    await Task.Delay(1000);
                    await apartment.Run(() => Application.ExitThread());
                }
            }
            catch (Exception ex)
            {
                Console.WriteLine($"Error: {ex.Message}");
                Environment.Exit(-1);
            }

            GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced);
            GC.WaitForPendingFinalizers();

            Console.WriteLine($"IsAlive: {WinformsApartment.s_debugTaskRef.IsAlive}");
            Console.ReadLine();
        }
    }

    public class WinformsApartment : IDisposable
    {
        readonly Thread _thread; // the STA thread

        readonly TaskScheduler _taskScheduler; // the STA thread's task scheduler

        readonly Task _threadEndTask; // to keep track of the STA thread completion

        readonly object _lock = new object();

        public TaskScheduler TaskScheduler { get { return _taskScheduler; } }

        public Task AsTask { get { return _threadEndTask; } }

        /// <summary>MessageLoopApartment constructor</summary>
        public WinformsApartment(Func<Form> createForm)
        {
            var schedulerTcs = new TaskCompletionSource<TaskScheduler>();

            var threadEndTcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

            // start an STA thread and gets a task scheduler
            _thread = new Thread(_ =>
            {
                try
                {
                    // handle Application.Idle just once
                    // to make sure we're inside the message loop
                    // and the proper synchronization context has been correctly installed

                    void onIdle(object s, EventArgs e) {
                        Application.Idle -= onIdle;
                        // make the task scheduler available
                        schedulerTcs.SetResult(TaskScheduler.FromCurrentSynchronizationContext());
                    };

                    Application.Idle += onIdle;
                    Application.Run(createForm());

                    threadEndTcs.TrySetResult(true);
                }
                catch (Exception ex)
                {
                    threadEndTcs.TrySetException(ex);
                }
            });

            async Task waitForThreadEndAsync()
            {
                // we use TaskCreationOptions.RunContinuationsAsynchronously
                // to make sure thread.Join() won't try to join itself
                Debug.Assert(Thread.CurrentThread != _thread);
                await threadEndTcs.Task.ConfigureAwait(false);
                _thread.Join();
            }

            _thread.SetApartmentState(ApartmentState.STA);
            _thread.IsBackground = true;
            _thread.Start();

            _taskScheduler = schedulerTcs.Task.Result;
            _threadEndTask = waitForThreadEndAsync();
        }

        // TODO: it's here for debugging leaks
        public static readonly WeakReference s_debugTaskRef = new WeakReference(null); 

        /// <summary>shutdown the STA thread</summary>
        public void Dispose()
        {
            lock(_lock)
            {
                if (Thread.CurrentThread == _thread)
                    throw new InvalidOperationException();

                if (!_threadEndTask.IsCompleted)
                {
                    // execute Application.ExitThread() on the STA thread
                    var terminatorTask = Run(() => Application.ExitThread());

                    s_debugTaskRef.Target = terminatorTask; // TODO: it's here for debugging leaks

                    _threadEndTask.GetAwaiter().GetResult();
                }
            }
        }

        /// <summary>Task.Factory.StartNew wrappers</summary>
        public Task Run(Action action, CancellationToken token = default(CancellationToken))
        {
            return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler);
        }

        public Task<TResult> Run<TResult>(Func<TResult> action, CancellationToken token = default(CancellationToken))
        {
            return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler);
        }

        public Task Run(Func<Task> action, CancellationToken token = default(CancellationToken))
        {
            return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler).Unwrap();
        }

        public Task<TResult> Run<TResult>(Func<Task<TResult>> action, CancellationToken token = default(CancellationToken))
        {
            return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler).Unwrap();
        }
    }
}

I suspect this might be a .NET Framework bug. I'm currently investigating it and I'll post what I may find, but maybe someone could provide an explanation right away.

noseratio
  • 59,932
  • 34
  • 208
  • 486
  • You need to `GC.Collect` again after `GC.WaitForPendingFinalizers` – canton7 Aug 23 '19 at 12:24
  • 1
    Can you move the `using` part, that is all the code in `try { ... }` into its own method and test again? I just want to eliminate the possibility that you have a hidden temporary variable introduced by the debugger or jitter keeping references alive. – Lasse V. Karlsen Aug 23 '19 at 12:24
  • @canton7 I tried that too and it didn't change anything. – noseratio Aug 23 '19 at 12:24
  • Have you confirmed that the `WindowsFormsSynchronizationContext` is actually installed at the point that you try to capture it btw? – canton7 Aug 23 '19 at 12:27
  • @LasseVågsætherKarlsen, good point, I just moved it to its own static TestAsync method and tried that, but it didn't change anything. – noseratio Aug 23 '19 at 12:28
  • Have you confirmed that the `() => Application.ExitThread()` is actually executed, by putting a breakpoint in it? – canton7 Aug 23 '19 at 12:29
  • canton7, I did and it's indeed. I do have a gut feeling it is still something specific to WindowsFormsSynchronizationContext, I'm trying something else related to that... – noseratio Aug 23 '19 at 12:30
  • 1
    (My suspicion is that you're telling the application to ExitThread twice. If the first one actually stops its message pump, then the second call (in `WinformsApartment.Dispose`) will never get scheduled. Therefore the Task that represents it will just hang around) – canton7 Aug 23 '19 at 12:32
  • @canton7, I have two `Application.ExitThread()` calls here. The one in `Dispose` doesn't get executed and that's expected (still the task should get GC'ed). If I comment out the one which is in `using`, then the second one gets executed and is `IsAlive` is false, but that's expected. – noseratio Aug 23 '19 at 12:33
  • @canton7 rather, it gets scheduled but never gets executed because the pump has stopped. Still should get GC'ed. – noseratio Aug 23 '19 at 12:34
  • 1
    "*still the task should get GC'ed*" -- I don't think that's true at all. The Task's operation is sitting on a message queue, but nothing's pumping that queue. The Task doesn't know that the thing pumping the queue has died - as far as it knows, the queue's just taking a while to get pumped. – canton7 Aug 23 '19 at 12:35
  • Hmmm, I see that you do wait for the thread to stop, which should kill its SynchronizationContext. Worth digging out a profile / SoS and finding out what's actually got a reference to that Task? It's possible that the WinformsApartment is still referenced and that's keeping the TaskScheduler alive? – canton7 Aug 23 '19 at 12:41
  • @canton7, I can confirm the `WindowsFormsSynchronizationContext.Current` is gone after `Application.Run(createForm())`, and I even tried `SynchronizationContext.SetSynchronizationContext(null)` explicitly there, so the queue should be gone too (and eventually GC'ed, and so is the task). I think I nearly figured it out, thanks to your input too! – noseratio Aug 23 '19 at 12:42
  • Actually, the SynchronizationContext just forwards things to a Control, so the SynchronizationContext itself won't be retaining anything. – canton7 Aug 23 '19 at 12:44
  • 1
    Nice class! Small stylistic change: I would rename the field `_threadEndTask` to `_completion`, and the property `AsTask` to `Completion`, following the example of a similar [Dataflow](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.dataflow.idataflowblock.completion) property. :-) – Theodor Zoulias Aug 24 '19 at 11:00
  • 1
    @TheodorZoulias thanks! it's a good suggestion :) – noseratio Aug 24 '19 at 11:11

1 Answers1

2

Ok so it appears the WindowsFormsSynchronizationContext doesn't get properly disposed of here. Not sure if it's a bug or a "feature", but the following change does fix it:

SynchronizationContext syncContext = null;

void onIdle(object s, EventArgs e) {
    Application.Idle -= onIdle;
    syncContext = SynchronizationContext.Current;
    // make the task scheduler available
    schedulerTcs.SetResult(TaskScheduler.FromCurrentSynchronizationContext());
};

Application.Idle += onIdle;
Application.Run(createForm());

SynchronizationContext.SetSynchronizationContext(null);
(syncContext as IDisposable)?.Dispose();

Now IsAlive is false and the task gets properly GC'ed. Comment out (syncContext as IDisposable)?.Dispose() above, and IsAlive is back to true.

Updated, if anyone uses a similar pattern (I myself use it for automation), I'd now recommend controlling the lifetime and disposal of WindowsFormsSynchronizationContext explicitly:

public class WinformsApartment : IDisposable
{
    readonly Thread _thread; // the STA thread

    readonly TaskScheduler _taskScheduler; // the STA thread's task scheduler

    readonly Task _threadEndTask; // to keep track of the STA thread completion

    readonly object _lock = new object();

    public TaskScheduler TaskScheduler { get { return _taskScheduler; } }

    public Task AsTask { get { return _threadEndTask; } }

    /// <summary>MessageLoopApartment constructor</summary>
    public WinformsApartment(Func<Form> createForm)
    {
        var schedulerTcs = new TaskCompletionSource<TaskScheduler>();

        var threadEndTcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

        // start an STA thread and gets a task scheduler
        _thread = new Thread(_ =>
        {
            try
            {
                // handle Application.Idle just once
                // to make sure we're inside the message loop
                // and the proper synchronization context has been correctly installed

                void onIdle(object s, EventArgs e)
                {
                    Application.Idle -= onIdle;
                    // make the task scheduler available
                    schedulerTcs.SetResult(TaskScheduler.FromCurrentSynchronizationContext());
                };

                Application.Idle += onIdle;
                Application.Run(createForm());

                threadEndTcs.TrySetResult(true);
            }
            catch (Exception ex)
            {
                threadEndTcs.TrySetException(ex);
            }
        });

        async Task waitForThreadEndAsync()
        {
            // we use TaskCreationOptions.RunContinuationsAsynchronously
            // to make sure thread.Join() won't try to join itself
            Debug.Assert(Thread.CurrentThread != _thread);
            try
            {
                await threadEndTcs.Task.ConfigureAwait(false);
            }
            finally
            {
                _thread.Join();
            }
        }

        _thread.SetApartmentState(ApartmentState.STA);
        _thread.IsBackground = true;
        _thread.Start();

        _taskScheduler = schedulerTcs.Task.Result;
        _threadEndTask = waitForThreadEndAsync();
    }

    // TODO: it's here for debugging leaks
    public static readonly WeakReference s_debugTaskRef = new WeakReference(null);

    /// <summary>shutdown the STA thread</summary>
    public void Dispose()
    {
        lock (_lock)
        {
            if (Thread.CurrentThread == _thread)
                throw new InvalidOperationException();

            if (!_threadEndTask.IsCompleted)
            {
                // execute Application.ExitThread() on the STA thread
                var terminatorTask = Run(() => Application.ExitThread());

                s_debugTaskRef.Target = terminatorTask; // TODO: it's here for debugging leaks

                _threadEndTask.GetAwaiter().GetResult();
            }
        }
    }

    /// <summary>Task.Factory.StartNew wrappers</summary>
    public Task Run(Action action, CancellationToken token = default(CancellationToken))
    {
        return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler);
    }

    public Task<TResult> Run<TResult>(Func<TResult> action, CancellationToken token = default(CancellationToken))
    {
        return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler);
    }

    public Task Run(Func<Task> action, CancellationToken token = default(CancellationToken))
    {
        return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler).Unwrap();
    }

    public Task<TResult> Run<TResult>(Func<Task<TResult>> action, CancellationToken token = default(CancellationToken))
    {
        return Task.Factory.StartNew(action, token, TaskCreationOptions.None, _taskScheduler).Unwrap();
    }
}
noseratio
  • 59,932
  • 34
  • 208
  • 486
  • @canton7, that's what it is. For more details, I'd need to go to .NET sources, but I imagine they don't call `WindowsFormsSynchronizationContext.Dispose` when they uninstall it from the thread upon/after `Application.ExitThread()`. – noseratio Aug 23 '19 at 12:50
  • `WindowsFormSynchronizationContext.Dispose` just disposes the control that it posts things to (which is probably your `Form`?). So chances are that it's the `Form` that isn't getting disposed? Which makes sense, as it's the `Form` that's holding the message queue here, I think. – canton7 Aug 23 '19 at 12:50
  • @canton7, I've changed the above to be `var form = createForm(); Application.Run(form); form.Dispose()` and it still doesn't work without calling `WindowsFormsSynchronizationContext.Dispose` explicitly. Looks like a bug to me. – noseratio Aug 23 '19 at 13:04
  • [See here](https://referencesource.microsoft.com/#System.Windows.Forms/winforms/Managed/System/WinForms/WindowsFormsSynchronizationContext.cs,70). You will also probably need to `SynchronizationContext.SetSynchronizationContext(null);` as well, as the SynchronizationContext holds a reference to the `Form`. – canton7 Aug 23 '19 at 13:10
  • Aha, although the `Dispose()` also sets `controlToSendTo = null;`, which suggests that something else might be retaining your `SynchronizationContext` – canton7 Aug 23 '19 at 13:10
  • @canton7, actually it works without `SynchronizationContext.SetSynchronizationContext(null)` as well, but `syncContext as IDisposable)?.Dispose()` is vital. – noseratio Aug 23 '19 at 14:19