How to prevent malfunction (memory leak) by dispos

Question

I am using ASP.NET 4.0.

From last few weeks, few users are complaining that the application starts malfunctioning. The GridView suddenly starts showing contents of a DropDown control that the same user or another concurrent user might have accessed at any point of time in that day. Similarly, DropDown controls may get populated by RowID of the any old result-set instead of the actual items.

I came across an article: Users seeing other users data in ASP.NET where the author discusses about Static objects responsible for memory leak behavior.

It reminds me of a class in my project which is Static and contains public static methods. This class contains methods to populate a DropDown, return a DataSet for a query input or return a scalar object based on a query input.

Extract of this class is below:

public static class reuse
{
    public static void FillDropDownList(string Query, DropDownList DropDownName, string ConnectionStringParameter)
    {
        SqlDataReader dr;

        try
        {
            dbConnection.OpenConnection(ConnectionStringParameter);

            //Check whether the Drop Down has existing items. If YES, empty it.
            if (DropDownName.Items.Count > 0)
                DropDownName.Items.Clear();

            SqlCommand cmd = new SqlCommand(Query,dbConnection.cn);
            dr = cmd.ExecuteReader();

            DropDownName.Items.Add("-- Select --");
            DropDownName.Items.Add("All");
            while (dr.Read())
                DropDownName.Items.Add(dr[0].ToString());

            dr.Close();
        }
        catch (Exception ex)
        {
            rpkCustomErrorHandler.GetScript(HttpContext.Current.Response,ex.Message.ToString());
        }
        dbConnection.CloseConnection();
    }
}

I want to know whether this is the cause of the malfunction I discussed above. If yes, is there any way to dispose Static Methods after the task of the method has finished. Do I need to change the class from Static to a default plain class?

Edited

I have another class which is also static and is used by the above class:

public static class dbConnection
{
    public static SqlConnection cn = new SqlConnection();

    public static void OpenConnection()
    {
        try
        {
            cn.ConnectionString = ConfigurationManager.ConnectionStrings["cnWebTwDrill"].ToString();

            if (cn.State == ConnectionState.Closed)
                cn.Open();
        }
        catch (Exception)
        {
            throw;              
        }
    }
}

Shall I remove "Static" from the Connection Class and make call to this class by using a unique instance each time?

Answer 1

Its not a question of leaking or disposing its a question of utilizing static objects with state. This function is static, but it is not statefull all of the context of the operation is present in the call as such all of the objects in this class that are not parameters will be cleaned up automatically. However if you defined some type of static data any users in the same app domain would share the value. That being said you must be carefully any time you utilize statics as they are hard to test replace and are very difficult in multithreading situations.

Answer 2

I doubt the error you're seeing is caused by a memory leak or a failure to Dispose of static objects.

However, to answer your question: the only way to free the memory held by static objects is to set the associated static references to null. After that, they won't be reachable, and the GC will take care of the rest.

Two things that might help with the actual problem:

1. Faulty caching strategies can cause unintentional data sharing (at the HTTP level or with the ASP.NET object cache, output caching, etc)
2. Be sure to use locking around all references to static objects that can be shared between threads, where one thread might write the object while another is reading it.

BTW, the static method in the OP does not appear to use any static objects; everything it touches seems to be passed as an argument. Static methods alone don't allocate or hold memory references.

Update:

Trying to share a single DB connection between page requests is a bug, and will almost certainly result in undefined behavior (for example, one page could issue a query, while another page reads the response). DB connections are already pooled by the runtime; you don't need to optimize that part on your own. Each page request should not only use a new SqlConnection, it should also call Dispose() on the connection object afterwards, to promptly release it back to the pool.

Answer 3

The code you've shown us is an example of bad usage of static class/method. You put in a static method or class things that are to simple to fit in an object AND which DON'T REQUIRE state.

The functionality you have there should be split between the Business Layer (which will retrieve the data) and the UI/COntroller (in this case Page ) where you assign the data to the server control. All this operations are specific to a request, there is no reason to use a static method for that. Is just a sign of (bad) procedural programming. And when dealing with db access (or disposable objects) you should use the using statement. Something like this

using(var conex=GetConnection())
{
   try
   {
     conex.Open(); 
     //do stuff

    var cmd= new SqlCommand();//cfg command
    using (var rd= cmd.ExecuteReader())   
      {
           //do read
      }
   }
   catch(Exception ex)
    {
       //handle exception
    }
}

The using statement automatically calls Dispose at the end of the block. It's basically a shortcut for

try {}
finally{ //dispose }.