David Sykes
David Sykes

Reputation: 49832

Why does my simple wpf C# app with a DispatcherTimer appear to leak memory?

I have created a simple wpf C# with a single text box and DispatcherTimer that displays the result of a call to GC.GetTotalMemory(true) every second. The value returned increases steadily with each call, and task manager shows that the private working memory set increases also. Is this really a memory leak, or just the appearance of one? In my real app that does a lot more within each tick the memory leakages appear significantly higher. My code is as follows

xaml

<Window x:Class="TestWPFApplication.Window1"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    Title="Window1" Height="300" Width="300">
    <Grid>
        <TextBox Name="memoryuseage"></TextBox>
    </Grid>
</Window>

xaml.cs

namespace TestWPFApplication
{
    /// <summary>
    /// Interaction logic for Window1.xaml
    /// </summary>
    public partial class Window1 : Window
    {
        System.Windows.Threading.DispatcherTimer tmr;

        public Window1()
        {
            InitializeComponent();

            tmr = new System.Windows.Threading.DispatcherTimer();
            tmr.Interval = new System.TimeSpan(0, 0, 1);
            tmr.Tick += new EventHandler(StaticTick);
            tmr.Start();
        }

        void StaticTick(object o, EventArgs sender)
        {
            memoryuseage.Text = GC.GetTotalMemory(true).ToString();
        }
    }
}

Upvotes: 5

Views: 1670

Answers (3)

BionicCode
BionicCode

Reputation: 28988

There is not enough information to tell if your application suffers from memory leaks. Only a profiler can tell. At least the code you have posted introduces a memory leak.

Actually, the code introduces two timer related memory leaks.

The first memory leak of the rendered scenario is a classic event handler leak. The special case here is that the timer instance (the event publisher or event source) is kept alive whenever a reference to a listener exists (registered event handler). In other words, closing or disposing the Window alone will not free the allocated system resource (as this is usually the case with other resources that are allocated using the memory of the current process).
And because the timer is kept alive, the event listeners are kept alive as well.

WPF timers use low-level system or OS timers which are unmanaged (that's why some implement IDisposable e.g., System.Threading.PeriodicTimer or System.Threading.Timer).
This unmanaged system resource is kept alive until the disposal of the referencing timer object or this timer object explicitly frees the timer resource e.g., when the DispatcherTimer is stopped.
Because the system timer is an unmanaged system resource that is explicitly kept alive by the framework, timer resources are not freed when the referencing object is disposed or has no incoming references.

To avoid this leak, it is crucial to unregister all event handlers (or in case of timers that implement IDisposable, Dispose() must be called).
Otherwise, the object instance (e.g. a Window) and its complete reference graph is kept alive by the event source (e.g. DispatcherTimer) - even when there is no strong reference to the object that owns the timer.

That's why we must always unsubscribe from timer events (and dispose those timers that implement IDisposable).
Unsubscribing from events when we no longer need to observe an object is in general common best practice. Had we followed this best practice the memory leak would have been avoided.

The documentation for the DispatcherTimer states:

"A DispatcherTimer will keep an object alive whenever the object's methods are bound to the timer."

Such a bound method is an event handler. For example, in a Window, simply override the Window.OnClosed method and unregister all event handlers and stop and dispose all timers.

The second memory leak is introduced by missing to stop the DispatcherTimer.
In this context, it's also important to note that the DispatcherTimer will continue to execute if it is not stoppped by one of the following events:

  • calling DispatcherTimer.Stop
  • setting DispatcherTimer.IsEnabled to false.

This also applies to other timers like the System.Threading-Timer etc.
When not stopped, the timer will continue to run even if the owner (e.g. a Window) has been garbage collected.

The DispatcherTimer is a simple wrapper around the system timer. Internally, the created system timer hooks are stored inside a global table. If the DispatcherTimer is not stopped, then the timer hook and it's wrapping DispatcherTimer is kept alive and continues to execute until the it is explicitly stopped.

This means this second leak exists even if all event handlers are unregistered and no strong reference to the owning object exists and the owner already got garbage collected as the timer continues to execute.

Summary

The first leak is keeping the window (or in general the object that owns the timer) and all its outgoing references alive. Unregistering all event handlers prevents this memory leak.

The second leak is the timer instance itself. Although the owner of the timer instance has been successfully garbage collected (because all event handlers were correctly unregistered), the timer continues to execute. This means unregistering event handlers is not enough, although it fixes the bigger leak. Stopping the timer prevents the memory leak.

The fixed for the code posted in the question can look as folowes:

public partial class Window1 : Window
{
  System.Windows.Threading.DispatcherTimer timerThatUsesunmanagedSystemResources;

  public Window1()
  {
    InitializeComponent();

    this.timerThatUsesunmanagedSystemResources = new DispatcherTimer();
    this.timerThatUsesunmanagedSystemResources.Interval = TimeSpan.FromSeconds(1);
    this.timerThatUsesunmanagedSystemResources.Tick += OnTick;
    this.timerThatUsesunmanagedSystemResources.Start();
  }

  private void OnTick(object sender, EventArgs e)
  {
     // TODO::Do something
  }
  
  protected override void OnClosed(EventArgs e) 
  { 
    base.OnClosed(e);

    this.timerThatUsesunmanagedSystemResources.Stop();

    // Unsubscribe ALL handlers from timer events
    // to avoid the event handler leak
    this.timerThatUsesunmanagedSystemResources.Tick -= OnTick;
  }
}

To prove the memory leak that is introduced by careless timer handling the following section presents the memory profiling results.
The code that was used for profiling can be found at the end of the section.

Profiler Results

The profiled application contains a simple main window that shows a second Window instance (MemoryLeakWindow) that runs a System.Threading.DispatcherTimer and a System.Threading.Timer.

Context

The profiling involves three scenarios:

Scenario #1: The MemoryLeakWindow closes without unsubscribing its event handlers from the System.Threading.DispatcherTimer.Tick event or disposes the System.Threading.Timer.

Scenario #2: The MemoryLeakWindow closes but unsubscribes its event handlers from the System.Threading.DispatcherTimer.Tick event or disposes the System.Threading.Timer.

Scenario #3: The MemoryLeakWindow closes and correctly unsubscribes its event handlers from the System.Threading.DispatcherTimer.Tick event. But it forgets to stop the timer, which is expected to cause the timer to continue to run after the MemoryLeakWindow has been garbage collected.

Analysis:

The first screenshot shows the retention paths for the MemoryLeakWindow of Scenario #1, how it is retained by the event handlers that were registered with the System.Threading.DispatcherTimer or System.Threading.Timer.

Because an event delegate always stores a strong reference to the event target (in this case MemoryLeakWindow) in order to invoke the callback, the target instance won't be eligible for garbage collection, if the event publisher's (e.g., System.Threading.DispatcherTimer or System.Threading.Timer) lifetime exceeds the lifetime of the event listener.

In this case, the System.Threading.DispatcherTimer or System.Threading.Timer is keeping the MemoryLeakWindow alive until there are no more registered event handlers, forced by the strong reference from the event delegate to the delegate's target:

enter image description here

The second screenshot shows how Scenario #2 allows the event listener to be successfully garbage collected after the MemoryLeakWindow had stopped the timers and unregistered its event handlers properly from the System.Threading.DispatcherTimer.Tick event or the System.Threading.Timer has been disposed. Therefore, the event handlers (or the event source to be more precise) are no longer creating a retention path:

enter image description here

Note: the MemoryLeakWindow instance is artificially kept alive by not clearing the strong reference in the MainWindow after closing the MemoryLeakWindow instance (e.g., by setting the TimerWindow instance variable to null).
I did this to show that the closed MemoryLeakWindow instance itself is no longer retained by the timers, as now visible in the retention path graph (this also does how important it is to properly control object lifetimes and clear instance variables to allow those references to become eligible for garbage collection). The graph shows that MemoryLeakWindow is now only kept alive by the MainWindow instance.

The third screenshot shows that how Scenario #3 is keeping the DispatcherTimer alive and that the timer is still running although all event handlers were previously unregistered and no strong reference to the owning object exists and the owner has already been garbage collected. This is because the timer continues to run:

enter image description here

The profiler shows that the DispatcherTimer instance has DispatcherTimer.IsEnabled still set to true, which means it is still running and consuming memory (second memory leak). Unless we stop the timer, it will run for the lifetime of the application.

Profiled Context

MainWindow.xaml.cs

partial class MainWindow : Window
{
  private MemoryLeakWindow TimerWindow { get; set; } 

  private void OnShowMemoryLeakWindowClicked(object sender, RoutedEventArgs e)
  {
    this.TimerWindow = new MemoryLeakWindow();
    this.TimerWindow.Show();
  }

  private void OnCloseMemoryLeakWindowWithoutUnregisterEventHandlersClicked(object sender, RoutedEventArgs e)
  {
    this.TimerWindow.IsUnregisterEventHandlersEnabled = false;
    this.TimerWindow.Close();

    // Although the strong reference is removed, the MemoryLeakWindow
    // and all its referenced objects are kept alive in memory
    // by the timer.
    this.TimerWindow = null;
  }

  private void OnCloseMemoryLeakWindowAndUnregisterEventHandlersClicked(object sender, RoutedEventArgs e)
  {
    this.TimerWindow.IsUnregisterEventHandlersEnabled = true;
    this.TimerWindow.Close();

    // Important: to allow the MemoryLeakWindow to get garbage collected
    // we must remove the strong reference 
    // e.g., by setting the instance or class variable to null.
    // This example is not doing this in order to keep the instance alive 
    // so that we can check with the memory profiler that the retention path of the timer is now removed.
    // The following line is deliberately not executed
    // for scenario #1 + #2. but executed for scenario #3
    //this.TimerWindow = null;
  }
}

MemoryLeakWindow.xaml.cs

public partial class MemoryLeakWindow : Window
{
  public bool IsUnregisterEventHandlersEnabled { get; set; }
  private DispatcherTimer dispatcherTimer;
  private Timer timer;

  public MemoryLeakWindow()
  {
    InitializeComponent();

    this.dispatcherTimer = new DispatcherTimer(
      TimeSpan.FromSeconds(1),
      DispatcherPriority.Input,
      OnDispatcherTimerTicked,
      Application.Current.Dispatcher);

    this.timer = new Timer(
      OnTimerTicked,
      null,
      TimeSpan.Zero,
      TimeSpan.FromSeconds(1));
  }

  protected override void OnClosed(EventArgs e)
  {
    base.OnClosed(e);

    if (this.IsUnregisterEventHandlersEnabled)
    {
      // If not stopped, the timer instance will leak
      // as it will continue to run
      this.dispatcherTimer.Stop();

      // If not unregistered, the event handlers will keep the MemoryLeakWindow alive
      this.dispatcherTimer.Tick -= OnDispatcherTimerTicked;

      // If not stopped, the event handlers will keep the MemoryLeakWindow alive
      _ = this.timer.Change(TimeSpan.Zero, Timeout.InfiniteTimeSpan);

      // If not disposed, the timer instance will leak
      this.timer.Dispose();
    }
  }

  private void OnTimerTicked(object? state)
  {
    this.Dispatcher.InvokeAsync(() =>
    {
      double currentProgressValue = this.TimerProgressBar.Value;
      this.TimerProgressBar.Value = currentProgressValue >= 60 
        ? 1 
        : currentProgressValue + 1;
    }, DispatcherPriority.Input);
  }

  private void OnDispatcherTimerTicked(object? sender, EventArgs e)
  {
    double currentProgressValue = this.DispatcherTimerProgressBar.Value;
    this.DispatcherTimerProgressBar.Value = currentProgressValue >= 60 
      ? 1 
      : currentProgressValue + 1;
  }
}

MemoryLeakWindow.xaml

<Window x:Class="Net.Wpf.MemoryLeakWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
        xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
        xmlns:local="clr-namespace:Net.Wpf"
        mc:Ignorable="d"
        Title="MemoryLeakWindow" Height="450" Width="800">
  <StackPanel>
    <ProgressBar x:Name="DispatcherTimerProgressBar"
                 Maximum="60" />
    <ProgressBar x:Name="TimerProgressBar"
                 Maximum="60"
                 Foreground="Blue" />
  </StackPanel>
</Window>

Upvotes: 1

user900202
user900202

Reputation:

I don't think there will be a memory leak in your codes. The memory increases just because of the GC.Collect() method is not called at real-time.

Try this:

void StaticTick(object o, EventArgs sender)
{
    GC.Collect();
    memoryuseage.Text = GC.GetTotalMemory(true).ToString();
}

Upvotes: 0

Henk Holterman
Henk Holterman

Reputation: 273294

Is this really a memory leak, or just the appearance of one?

Just the appearance. A steady increase is normal, it isn't a leak until you can crash it by running it long enough. But that could take days with a small leak.

Your memory usage ought to level off, but only after a considerable time. You might be able to speed that up by using GC.Collect() here (every 10th tick or so).

For a serious diagnosis you will need a memory-profiler.

For example: http://en.wikipedia.org/wiki/CLR_Profiler

Upvotes: 2

Related Questions