Follow @PDSAInc Use Close and Finally - Paul Sheriff's Blog for the Real World

Paul Sheriff's Blog for the Real World

This blog is to share my tips and tricks garnered over 25+ years in the IT industry

Paul's Favorites

Use Close and Finally

In the last 2 weeks I have had two different clients complain that there are "memory leaks" in .NET. I tell them very politely, that most likely it is their code!<g> In both cases it was their code. The first case had to do with the programmers using a DataReader and not closing them when they were done with them, or they put the Close() method call within their Try block and not in a Finally. The second case involved the StreamWriter in the File.IO namespace where once again the file was not always being closed correctly due to the programmer not putting the close in the Finally block. Below is what the original code looked like: 

using System.IO;
private void CreateLogFile()
{
  try
  {
    StreamWriter sw =
      new StreamWriter(@"D:\Samples\Test.txt",
      true, System.Text.UTF8Encoding.UTF8);

    sw.WriteLine("This is some text");
    sw.Close();
  }
  catch(Exception ex)
  {
    throw ex;
  }
}


You can see in the above code that the sw.Close() method is within the Try block. If an exception occurs when trying to open or write to the file, the code would go immediately to the Catch block and the Close would never execute. If this method is being called many times, this can cause and "apparent" memory leak. With just a little re-factoring the code was fixed to close the file in the Finally block.

private void CreateLogFile()
{
  StreamWriter sw = null;

  try
  {
    sw = new StreamWriter(@"D:\Samples\Test.txt",
      true, System.Text.UTF8Encoding.UTF8);

    sw.WriteLine("This is some text");
  }
  finally
  {
    if (sw != null)
    {
      sw.Close();
    }
  }
}

Notice that there are three items that we had to fix up here. First, the declaration of the StreamWriter was moved out of the Try block. If we did not do this, then the variable "sw" would not able to be accessed from within the Finally block since it would have block-level scope only within the Try portion. Secondly, we created a Finally block, checked the "sw" variable for null, and if it was not null then we closed it. Thirdly, since nothing was happening with the catch, we eliminated it all together.

Watch these types of scenarios as this can cause hard-to-find bugs in your code.

Posted: Jan 16 2007, 03:50 PM by psheriff | with 6 comment(s)
Filed under: ,

Comments

anonoymous said:

why not use "using"?

private void CreateLogFile()

{

 using(StreamWriter sw = new StreamWriter(@"D:\Samples\Test.txt",

     true, System.Text.UTF8Encoding.UTF8))

   sw.WriteLine("This is some text");

 }

}

# January 16, 2007 4:20 PM

psheriff said:

Sure, that works as well. You have to make sure that the object you are putting in the using clause has implemented the IDisposable interface. So it may not be applicable for all objects, but the StreamWriter does implement this interface (or actually the TextWriter from which it inherits does) so this works well.

# January 16, 2007 4:37 PM

psheriff said:

Note to VB.NET users. You can also use the "using" clause now in VB.NET 2.0!

# January 16, 2007 4:39 PM

AA said:

Use "Using" whenever we can, of course!

# January 16, 2007 4:42 PM

Dean Harding said:

Why do you have:

catch(Exception ex)

{

   throw ex;

}

? I would hope it's because in the "real" code you're doing more than just re-throwing the exception. But even then, your code should read:

catch(Exception ex)

{

 // Do stuff

 throw; // Does not blow away the stack trace

}

But even that is not correct. You should not be catching the Exception object at all. AccessViolationException inherits from Exception, and when you get an access violation, the only SAFE thing to do is to let the application terminate (i.e. DO NOT try to catch it, even if you re-throw it again). Instead, you should have:

catch(IOException ex)

{

 // Do stuff

 throw;

}

Finally, if you're really NOT going to have anything where I've got // Do stuff, then you don't need the catch clause at all:

try

{

 // Do stuff

}

finally

{

 // Do other stuff

}

is perfectly legal.

# January 16, 2007 6:06 PM

psheriff said:

Dean,

Yep, obviously my code was "fake code" just to show the technique. There are many ways to catch and handle exceptions (probably as many ways as there are programmers<g>). There are a lot of good articles out there. Here is one that I tend to agree with if you are not doing something with the caught exception.

http://codebetter.com/blogs/karlseguin/archive/2006/04/05/142355.aspx

# January 16, 2007 6:52 PM

Fabrice Marguerie said:

My trackback didn't appear here, so here goes:

http://weblogs.asp.net/fmarguerie/archive/2007/01/16/exception-handling-and-resource-protection-with-try-finally.aspx

The problem with "fake code" is that it gets copied and pasted all over. I hate it when I see this kind of code in official documentation for example because people often use it as a reference. Sample code should be as clean as possible.

# January 16, 2007 7:25 PM

mluker said:

private void ParseSiteFile(string pSiteFilePath){

           string LineValue = "";

           StreamReader sr = null;

           try

           {

               if (File.Exists(pSiteFilePath))

               {

                   sr = new StreamReader(pSiteFilePath);                      

               }

               else

               {

                   throw new Exception("Input file does not exist: " + pSiteFilePath);

               }

           }

           catch (Exception ex)

           {                

               throw;

           }finally{

               sr.Close();

               sw.Close();

           }

       }

}

# October 24, 2007 10:38 AM