Home > .NET, Application Design > Shared events and memory leaks

Shared events and memory leaks

I had something happen to an application I recently had been refactoring that resulted in a very serious memory leak. I work mostly with single-thread, data-driven applications and so rarely create the opportunity to fall into such traps given that the .NET framework manages garbage collection so well… actually, given that the framework manages garbage collection so well when you don’t try to get too tricky. In fact, I can’t recall even finding a memory leak in any previous applications in the past six years — though it’s plausible that I just never knew of one if it did exist. But this one was very obvious and the solution pointed to an almost unforgivable coding sin that I definitely should have caught. So if you ever have found yourself creating and using shared (static in C#) events then you may want to read on for a cautionary tale.

This project was for internal users to perform billing and invoicing functionality and had recently been released as version 1.0 to my company’s billing group. It contained had a component class called ContextManager that would be responsible for maintaining the user’s “context” in the single-user application – a sort of one-stop shop for everything user-contextual. Among other various functions this class maintained the “client context”, providing exclusive access to the client within which the user’s actions would contextually be run. Since there are around 100 potential clients and the user is expected to switch between these clients repeatedly while the application is running, such a component was designed to encapsulate the required logic behind these client switches and make accessible an instance of a client business object named CurrentClient to perform operations upon. I exposed an event called ClientChanged that would be raised whenever the user “moved” from one client to another. There were multiple forms in the project with controls that needed to change their state based upon this event and it was deemed critical that all displayed forms be synchronized in their client context. To facilitate this I chose to have an instance of ContextManager on each of those forms, then a Context_ClientChanged event handler was wired up in each form to refresh data grids, change labels accordingly, or any perform any other context-related duty.

When the user changed client context this action would have to be requested through the ContextManager. Doing this through an instance of the class (through a SetClient() method) seemed the obvious choice, but wait… how do I get this change propogated to all the other instances – remember all the displayed forms needed to be synchronized? Well, first I chose to change that SetClient method to being shared rather than on the instance. This meant that the form didn’t need to have an instance of ContextManager through which to set the client but the event heralding this change would be handled through an instance. This separation of scope was accomplished by creating a private shared event that all instances of ContextManager were given handlers to in their contructors. These handlers essentially bubbled up the event through their own public event that the forms then handled.

Here is an oversimplified version of the code to illustrate this model for those who need to see it rather than read about it:

Public Class Client
    Public Property ID As Int32
    Public Property Name As String

    Public Sub New(client_Id As Int32)
        '... perform data retrieval to get values for this ID...
    End Sub

    Public Function GetClientInfo() As DataTable
        '...perform data retrieval logic here and pass back some client-specific data.
    End Function

End Class
Public Class ContextManager

    Private _CurrentClient As Client

    Public ReadOnly Property CurrentClient As Client
        Get
            Return _CurrentClient
        End Get
    End Property

    Private Shared Event ClientChanging(newClient As Client)
    Public Event ClientChanged(sender As ContextManager, newClient As Client)

    Public Shared Sub SetClient(client_ID As Int32)
        Dim newClient As New Client(client_ID)
        'Raise the shared event so the instances will all register the client change.
        RaiseEvent ClientChanging(newClient)
    End Sub

    Public Sub New()
        'Ensure that all instances listen to the shared event.
        AddHandler ClientChanging, AddressOf OnClientChanging
    End Sub

    Protected Sub OnClientChanging(newClient As Client)
        'Bubble up the shared event to subscribers to this instance's event.
        RaiseEvent ClientChanged(Me, newClient)
    End Sub

End Class
Public Class Form1
    Private WithEvents Context As New ContextManager()

    Private Sub Context_ClientChanged(sender As ContextManager, newClient As Client) Handles Context.ClientChanged
        'Adjust controls to this new client...
        Me.dgvClient.DataSource = newClient.GetClientInfo
    End Sub

End Class

While it may seem to be an overly complicated design the code was relatively simple to get working and it was running flawlessly for the four months it had been in production. The problem, however, came when I decided to change this model for the upcoming next version release. I decided that I wanted to shift away from instances of ContextManager on each form and instead have all of the forms deal with client context from a single, shared set of functionality on the class.  Really, I was uncomfortable with the distribution of events to multiple instances and wanted to abbreviate this process.  This meant a few changes needed to be made:

  • The ClientChanged event would be changed to a Shared scope and the shared ClientChanging event would go away.
  • The entire set of members on the ContextManager class would be changed to a Shared scope.
  • Any call to CurrentClient on forms would need to be changed to call the property directly on the class name (including SetClient).
  • The  handlers of the ClientChanged event on the various forms would need to be changed over to referencing the shared event. This required adding the handlers manually at runtime.

Here is the revised code after these changes (note that the Client class did not need to change):

Public Class ContextManager

    Private Shared _CurrentClient As Client

    Public Shared ReadOnly Property CurrentClient As Client
        Get
            Return _CurrentClient
        End Get
    End Property

    Public Shared Event ClientChanged(newClient As Client)

    Public Shared Sub SetClient(client_ID As Int32)
        Dim newClient As New Client(client_ID)
        RaiseEvent ClientChanged(newClient)
    End Sub

    Private Sub New()
        'Scope prohibits instantiation of this class.
    End Sub

End Class
Public Class Form1

    Public Sub New()
        InitializeComponent()
        AddHandler ContextManager.ClientChanged, AddressOf ContextManager_ClientChanged
    End Sub

    Private Sub ContextManager_ClientChanged(newClient As Client)
        'Adjust controls to this new client...
        Me.dgvClient.DataSource = newClient.GetClientInfo
    End Sub

End Class

On the face of it we would seem to have a cleaner, more simplified design — and I am still happy with the choice. However, the savvy reader will notice the serious bug that was introduced into this code. To illustrate the issue lets just for the fun of it throw a button on to our dummy Form1. (For those that want to play along, the above code can be easily copied into the code-behind of Form1 on a fresh WinForms project – you just might want to either add a datagridview to that form or just comment out the line that references it). Here’s the code for that button’s Click event handler:

    Private Sub Button1_Click(sender As System.Object, e As System.EventArgs) Handles Button1.Click
        Using frm As New Form1()
            frm.ShowDialog()
        End Using

        ContextManager.SetClient(123)
    End Sub

Run the project and be sure to pin the Immediate Window on VS (possibly the Output Window if you have redirected debug information there). Now, every time you click the button just immediately close the dialog form that appears using the red X close button. Be sure you watch what appears in the debug window — you should see that with each form that is closed you get one extra “Client Changed” line. So the first time you close the dialog form you get two lines. The next you get three, then four, etc.  Here’s what you should see:

Debug ouput

As you might imagine this could be a problem for an application that is run all day long and is expected to have dialog forms popped open many dozens of times during the application’s life span. The trouble is compounded when these events precipitate expensive data calls that would now be called many times over. In fact, the lag-time in filling my grid was the clue that revealed the issue. It should be noted that these changes never made it into production or even QA, where they certainly would have been picked up should I have dropped the ball even further during development.

So why? What’s the difference that caused this problem? Recall that when we had the instance of ContextManager on the form that the event handler was tied to an event delegate on that instance. When the form was closed and disposed that ContextManager instance is set to a null reference and so the reference for the event handler is released to garbage collection. In the refactored version I have changed those event handlers to now reference the event on the shared ContextManager class — an assembly-managed reference that does not get nulled out when the form is disposed. Therefore those event handlers retain their pointer references and cannot be cleared for garbage disposal. The very serious ramifications are that each of those dialog forms never actually gets properly disposed of, so the event continues to be handled on perpetually-disposing but never disposed forms, at least until the app’s process is killed. And that is how one can cause a killer memory leak by using shared members of a class, in this case a shared event.

This episode provides a painful example of what should be considered a cardinal sin in coding: adding a handler without ensuring it is properly removed. I definitely knew this rule but am ashamed to say that I let it slip my mind when it mattered. In fact, a good rule of thumb is to take care of removing the handlers at the same time that you add them lest you forget later. In this case the solution was a quite easy fix — remove the handlers in the Disposed event of the form, like this:

    Private Sub Form1_Disposed(sender As Object, e As System.EventArgs) Handles Me.Disposed
        RemoveHandler ContextManager.ClientChanged, AddressOf ContextManager_ClientChanged
    End Sub

Now you know. Don’t make this mistake.

Advertisements
Categories: .NET, Application Design
  1. No comments yet.
  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: