0

I have this code, that runs every time a "primary menu" item is clicked. It basically fills out a submenu stackpanel with "buttoned" user controls.

    private void LoadSubMenuControls(int _menuid)
    {
        double _itemcount = 0;
        double _buttonHeight = 61;
        //fill out user permissions
        //get information  from users
        //release events and bindintgs
        foreach (Object _child in Midmenu.Children)
        {
            ((MenuButton)_child).MidMousePressed -= new MenuButton.MidMenuMouseButtonPressed(MenuButton_MidMousePressed);
            ((MenuButton)_child).DataContext = null;
        }
        Midmenu.Children.Clear();
        midScroll1.ScrollToTop();
        Midmenu.Height = MidHeight;
        Midmenu.Width = this.Width / 3;
        foreach (DataRow _row in _rxworksmenues.GetMenuItems(_menuid))
        {
            _itemcount++;
            MainScreenWPFUC.MenuButtonViewModel _butview = new MenuButtonViewModel();
            _butview.ApplyViewModel(_row,_itemcount);
            _butview.MenuButton.DataContext = _butview;
            _butview.MenuButton.Height = _buttonheight;
            _butview.MenuButton.Width = Midmenu.Width;
            _butview.MenuButton.MidMousePressed += new MenuButton.MidMenuMouseButtonPressed(MenuButton_MidMousePressed);
            Midmenu.Children.Add(_butview.MenuButton);
            Midmenu.UpdateLayout();
        }
        //When there are more items than height allows display the 
        //scrolbar and hide the border
        //also squeeze the midframe to accomodate scrollbar with width of 56 px
        if (MidHeight < _rxworksmenues.CountMenuItems(_menuid) * _buttonHeight)
        {
            midScroll1.VerticalScrollBarVisibility = ScrollBarVisibility.Visible;
            Midmenu.ScrollOwner = midScroll1;
            Midmenu.Width = this.Width /3 - 56;
            ScrollViewerBorder.BorderThickness = new Thickness(1,0,0,0);
        }
        else
        {
            midScroll1.VerticalScrollBarVisibility = ScrollBarVisibility.Hidden;
            ScrollViewerBorder.BorderThickness = new Thickness(1,0,1,0);
        }


        //TFS 10560
        //Forces garbage collection to rpevent memory link occuring in this function

        //System.Threading.Thread.Sleep(200);
        //GC.Collect();
        //GC.WaitForPendingFinalizers();

    }

What I've noticed happening is that every time this code is executed the memory on the process goes up several K and never lets down. What I've discovered from reading is that forcing garbage collection can help, and it does, but I've also read it's not a good practice and want to find where the leak actually occurs.

To state a question, how can I capture the leak, and is the code the cause of it?

Thank you.

GetMenuItems comes from this class.

 public class Menues
    {
        private DataTable _table = new DataTable("menues");

        public Menues()
        {
        }

        public void Fill(int sysuserid, string dbconnection)
        {
//            DateTime start = new DateTime(DateTime.Now.Ticks);
            SqlConnection _conn = new SqlConnection( dbconnection);
            SqlCommand _comm = new SqlCommand("Get_RxWorks_Menues",_conn);
            _comm.CommandType = CommandType.StoredProcedure;
            SqlParameter _param = new SqlParameter("@sysuserid",sysuserid);
            _comm.Parameters.Add(_param);
            SqlDataAdapter _da = new SqlDataAdapter(_comm);
            _da.Fill(_table);
 //           DateTime end = new DateTime(DateTime.Now.Ticks);
 //           MessageBox.Show(end.Subtract(start).TotalMilliseconds.ToString());
        }
        /// <summary>
        /// Fetches main menu items when parentid is ommited
        /// or submenu items when int parentid is specified
        /// </summary>
        /// <returns></returns>
        public DataRow[] GetMenuItems()
        {

            return _table.Select("parentid=0");

        }
        /// <summary>
        /// Fetches main menu items when parentid is ommited
        /// or submenu items when int parentid is specified
        /// </summary>
        /// <returns></returns>

        public DataRow[] GetMenuItems(int parentid)
        {
            return _table.Select("parentid=" + parentid);
        }

        /// <summary>
        /// Returns number of menu items, when empty returns left menu
        /// </summary>
        /// <returns></returns>
        public int CountMenuItems()
        {
                return _table.Select("parentid=0").Length;
        }
        /// <summary>
        /// Returns number of menu items, when empty returns left menu
        /// </summary>
        /// <returns></returns>
        public int CountMenuItems(int parentid)
        {
            return _table.Select("parentid=" + parentid).Length;
        }

        public DataTable MenuTable
        {
            get
            {
                return _table;
            }
        }
KonB
  • 220
  • 1
  • 10
  • Can you confirm how you determine that memory usage is rising? Do you look in Task Manager by any chance? – Daniel Kelley Apr 24 '14 at 15:36
  • You can use a memory profiler to detect the leakage. Take a look on the following SO question for good profilers. http://stackoverflow.com/questions/3927/what-are-some-good-net-profilers – Ikaso Apr 24 '14 at 15:40
  • Can you post the implementation of your `GetMenuItems` method? – Troy Carlson Apr 24 '14 at 15:40
  • Daniel, yes I go through the Task Manager, unfortunately at the moment I am not very skilled at .NET memory manager. Good opportunity to learn. – KonB Apr 24 '14 at 15:41
  • 3
    Forcing garbage collection does NOT prevent memory leaks. It only forces the GC to collect sooner than it would normally collect. If there truly is a memory leak, GC.Collect will not catch it. Just because the memory of your process is going up does not mean there is a memory leak. The GC is smart, and will not waste time collecting garbage or releasing memory until it has to or there is idle time to do so. – Nathan A Apr 24 '14 at 15:44
  • Troy, GetMenuItems comes back from this class, i'll post update in the main body.. – KonB Apr 24 '14 at 15:45
  • 1
    @user2687092 This question won't answer your question, but it does highlight the dangers of using TM to monitor memory usage in .Net applications - http://stackoverflow.com/questions/6331389/c-sharp-thread-not-releasing-memory. – Daniel Kelley Apr 24 '14 at 15:50
  • Daniel, it partially answers my question. Like i said before, I am not terribly familiar with these concepts yet, and I am simply reacting to what my eyes see, memory usage going up every time a click happens, and unfortunately what my QAs see. if I can educate them and myself on correct identification of memory leaks, and actual problems I can resolve this issue, without writing additional code. – KonB Apr 24 '14 at 15:54
  • 2
    Start here: http://msdn.microsoft.com/en-us/library/0xy59wtx(v=vs.110).aspx. Read up on the basics of GC and how to properly identify memory leaks. It's a very good thing to know if you're going to spend a lot of time programming in .NET. – Nathan A Apr 24 '14 at 15:56
  • 1
    I would use a memory profiler to ensure that you have a memory leak and that it is simply that the garbage collector has not yet collected. Even if you have not used a memory profiler before I would highly recommend it as it really is the only decent way of diagnosing these kinds of issues. Explicitly calling GC.Collect(); is not a good idea and should be avoided for 99% of scenario's. – BenjaminPaul Apr 24 '14 at 15:57
  • Where do you dispose objects? You add `MenuButton` and subscribe to events, which afaik makes hard references (so all your buttons you ever create will leave until application is closed). Not a single `using() {...}`. I don't see how often you use `Menues.Fill`, but connection to database have to be closed. Or you will have memory leaks. – Sinatr Apr 24 '14 at 16:06
  • Menues.Fill is used only once in another part of the code, it sits in memory until app is closed. MenuButtons and events i clear up the in the function LoadSubMenuControls, foreach (Object _child in Midmenu.Children) { ((MenuButton)_child).MidMousePressed -= new MenuButton.MidMenuMouseButtonPressed(MenuButton_MidMousePressed); ((MenuButton)_child).DataContext = null; } Midmenu.Children.Clear(); – KonB Apr 24 '14 at 16:12
  • I've implemented using through several instances and that sees to be working out very well. I referenced idisposable interface on my user controls as well as got rid of the UpdateLayout() code, don't need one. One question remains is whether or I need i need to make sure unmanaged resourced get disposed of. The only I i can think of are the resourcedictionaries that are referenced in the XAML in front of the button class. – KonB Apr 24 '14 at 18:12

1 Answers1

0

In your code:

public void Fill(int sysuserid, string dbconnection)
        {
//            DateTime start = new DateTime(DateTime.Now.Ticks);
            SqlConnection _conn = new SqlConnection( dbconnection);
            SqlCommand _comm = new SqlCommand("Get_RxWorks_Menues",_conn);
            _comm.CommandType = CommandType.StoredProcedure;
            SqlParameter _param = new SqlParameter("@sysuserid",sysuserid);
            _comm.Parameters.Add(_param);
            SqlDataAdapter _da = new SqlDataAdapter(_comm);
            _da.Fill(_table);
 //           DateTime end = new DateTime(DateTime.Now.Ticks);
 //           MessageBox.Show(end.Subtract(start).TotalMilliseconds.ToString());
        }

The SqlConnection is not closed. If the SqlConnection goes out of scope, it won't be closed. Therefore, you must explicitly close the connection by calling Close or Dispose. Close and Dispose are functionally equivalent.

If the connection pooling value Pooling is set to true or yes, the underlying connection is returned back to the connection pool. On the other hand, if Pooling is set to false or no, the underlying connection to the server is actually closed.

This could be the reason why the memory goes up.

To use SqlConnection properly, you can follow this:

using (SqlConnection connection = new SqlConnection(connectionString))
    {

        // Do work here; connection closed on following line.
    }

Please read the MSDN for details

Please also check if the UI objects you're using should be disposed explicitly as UI object will allocate native handles.

Matt
  • 6,010
  • 25
  • 36
  • Thank you Matt for noticing this, a rookie mistake made in a hurry, however this function is executed only once during the life of the application, I wouldn't expected it to be the reason. – KonB Apr 24 '14 at 16:24