Memory Leak caused by Task in ViewModel

Question

I have the following code, it causes a memory leak.

The problem is the task, when I remove it, everything is fine and the View as well as the ViewModel are GCed. It seems like the Task is keeping a reference to UpdateTimeDate and hence the ViewModel. I tried various things, but none have worked, hoping someone has any idea or explanation why it is the case.

public class HeaderViewModel : Observable, IDisposableAsync
{
    public HeaderViewModel (IDateTimeProvider dateTimeProvider)
    {
        TokenSource = new CancellationTokenSource();

        ATask = Task.Run(
            async () =>
            {
                while(!TokenSource.Token.IsCancellationRequested)
                {
                    UpdateTimeData();
                    await Task.Delay(800);
                }

                IsDisposed = true;
            },
            TokenSource.Token);

        UpdateTimeData();

        void UpdateTimeData()
        {
            TimeText = dateTimeProvider.Now.ToString("HH:mm:ss");
            DateText = dateTimeProvider.Now.ToString("dd.MM.yyyy");
        }
    }

    public CancellationTokenSource TokenSource { get; set; }

    public bool IsDisposed { get; set; }

    public string TimeText
    {
        get => Get<string>();
        set => Set(value);
    }

    public string DateText
    {
        get => Get<string>();
        set => Set(value);
    }

    private Task ATask { get; set; }

    public async Task Dispose()
    {
        TokenSource.Cancel();

        while(!IsDisposed)
        {
            await Task.Delay(50);
        }

        TokenSource.Dispose();
        ATask.Dispose();
        ATask = null;
        TokenSource = null;
    }
}

This is the Timer based solution, it also causes a memory leak:

public class HeaderViewModel : Observable, IDisposableAsync
{
    public HeaderViewModel(IDateTimeProvider dateTimeProvider)
    {
        DateTimeProvider = dateTimeProvider;

        ATimer = new Timer(800)
        {
            Enabled = true
        };

        UpdateTimeData(this, null);

        ATimer.Elapsed += UpdateTimeData;
    }

    public string TimeText
    {
        get => Get<string>();
        set => Set(value);
    }

    public string DateText
    {
        get => Get<string>();
        set => Set(value);
    }

    public bool IsDisposed { get; set; }

    private IDateTimeProvider DateTimeProvider { get; }

    private Timer ATimer { get; }

    public async Task Dispose()
    {
        ATimer.Stop();

        await Task.Delay(1000);

        ATimer.Elapsed -= UpdateTimeData;
        ATimer.Dispose();
        IsDisposed = true;
    }

    private void UpdateTimeData(object sender, ElapsedEventArgs elapsedEventArgs)
    {
        TimeText = DateTimeProvider.Now.ToString("HH:mm:ss");
        DateText = DateTimeProvider.Now.ToString("dd.MM.yyyy");
    }
}

Yes I tried a Timer. It has the exact same problem. I know that this particular piece of code causes the memory leak, because I analysed it with dotMemory and also have a test that instantiates the View multiple times. The memory keeps growing, unless the task/timer is removed. — Wadim Smirnow, Sep 04 '19 at 13:36
You are capturing an instance of the ViewModel (this) implicit in the lambda. Try to use a WeakRefrence instead. — keuleJ, Sep 04 '19 at 13:36
It's included now. I really don't understand why neither is working. — Wadim Smirnow, Sep 04 '19 at 13:49
Why does the `Timer` based solution need to implement `IDisposableAsync` (vs say `IDisposable`)? Have you put a breakpoint in the `Dispose` method of the `Timer` solution to validate whether it is called or not? — mjwills, Sep 04 '19 at 13:56
The memory leak depends on the view retained. This leak is fairly small but a simple enough example to illustrate the problem. The timer does not have to implement IDisposableAsync. I was just too lazy to change the interface and also added a delay to make sure the timer has stopped before disposing. I did try it with Disposable as well and also checked if all the code paths are hit and they are. — Wadim Smirnow, Sep 04 '19 at 14:12

Wadim Smirnow · Answer 1 · 2019-09-05T11:25:24.757

I found a solution. Thanks to keuleJ, he posted the comment that lead me to it. So the Task or Timer is capturing an instance of the ViewModel when you create either of them. The way to prevent it is to make a WeakReference and use that:

public class HeaderViewModel : Observable, IDisposableAsync
{
    public HeaderViewModel(IDateTimeProvider dateTimeProvider)
    {
        DateTimeProvider = dateTimeProvider;

        UpdateTimeData();

        var weakReference = new WeakReference(this);

        Task.Run(
            async () =>
            {
                while(!((HeaderViewModel)weakReference.Target).IsDisposing)
                {
                    ((HeaderViewModel)weakReference.Target).UpdateTimeData();
                    await Task.Delay(800);
                }

                ((HeaderViewModel)weakReference.Target).IsDisposed = true;
            });
    }

    public bool IsDisposed { get; set; }

    public string TimeText
    {
        get => Get<string>();
        set => Set(value);
    }

    public string DateText
    {
        get => Get<string>();
        set => Set(value);
    }

    private IDateTimeProvider DateTimeProvider { get; }

    private bool IsDisposing { get; set; }

    public async Task Dispose()
    {
        IsDisposing = true;

        while(!IsDisposed)
        {
            await Task.Delay(50);
        }
    }

    private void UpdateTimeData()
    {
        TimeText = DateTimeProvider.Now.ToString("HH:mm:ss");
        DateText = DateTimeProvider.Now.ToString("dd.MM.yyyy");
    }
}

Note that I also could not make a local variable out of

(HeaderViewModel)weakReference.Target

As soon as I did that some magic seems to happen and the instance would be captured again.

score 0 · Answer 2 · answered Sep 04 '19 at 15:37

0

It appears that your Dispose task never returns which is why your object is remaining in memory. I tracked down the issue to the

await Task.Delay(1000)

if you change it per this post https://stackoverflow.com/a/24539937/3084003 it will work

await Task.Delay(1000).ConfigureAwait(false);

answered Sep 04 '19 at 15:37

bwing

731
8
12

Hi there, thank you for the suggestion. Unfortunately that did not do the trick for me. I guess I'll devote another day to looking into this issue. – Wadim Smirnow Sep 05 '19 at 07:00

Memory Leak caused by Task in ViewModel

2 Answers2