Reputation: 49832
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
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:
DispatcherTimer.Stop
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.
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.
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
.
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.
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:
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:
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:
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.
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
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
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