hunting down a memory leak

[jason]

my w3wp.exe process has recently gotten an ever-expanding memory
footprint. the two most likely causes are either a memory leak in the
object library it uses, or session variables not getting cleaned up
correctly. this post is to check one thing in the object library, to
make sure that the "using" statement is being used correctly.

yes, i know that the use of error messaging is inadvisable and
unnecessary in .NET languages. we do plan to migrate this to use
exceptions in the future, however right now 90% of the consumers of
this object library are non.NET applications which cannot handle the
..NET exception classes.

public override bool LoadByID(string sID, string sConnection)
{
p_nErrorCode = 0;
p_sErrorMessage = "";

if (AuthorityID = Guid.Empty) return false;

bool bReturn = true;

using (SqlConnection oConn = new SqlConnection(sConnection))
using (SqlCommand oCommand =
this.CreateSqlCommand("parameterlist.xml", oConn))
{
try { oConn.Open(); }
catch (Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = "Error opening DB conenction during .DBLoadByID:
" + ex.Message;
goto Failed;
}
oCommand.Parameters["@objectid"].Value = new Guid(sID);
oCommand.Parameters["@authorityid"].Value = p_gAuthorityID;

try { oCommand.ExecuteNonQuery(); }
catch (Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = "Sql exception during LoadByID: " + ex.Message;
goto Failed;
}
if (Convert.IsDBNull(oCommand.Parameters["@rowcount"].Value))
{
p_nErrorCode = 10;
p_sErrorMessage = "Null rowcount during LoadByID.";
goto Failed;
}

int nEffect =
Convert.ToInt32(oCommand.Parameters["@rowcount"].Value);
if (nEffect == 1)
{
p_gID = new Guid(sID);
p_dtDateCreated =
GetDateTimeParameter(oCommand.Parameters["@datecreated"]);
p_dtDateModified =
GetDateTimeParameter(oCommand.Parameters["@datemodified"]);
p_gModifiedBy = (Guid)oCommand.Parameters["@modifiedby"].Value;

p_bLoaded = true;
}
else
{
p_nErrorCode = 10;
p_sErrorMessage = "Unexpected rowcount (" + nEffect + ") during
LoadByID.";
goto Failed;
}
Failed:
if (ErrorCode > 0) bReturn = false;
}
return bReturn;
}
and here is the stuff behind the function call above
"CreateSqlCommand":

public SqlCommand CreateSqlCommand(string sFileName, SqlConnection
oConn)
{
XMLCommand oXMLCommand = new XMLCommand();
SqlCommand oCommand = oXMLCommand.CreateSqlCommand(sFileName);
if (oCommand == null)
{
p_nErrorCode = oXMLCommand.ErrorCode;
p_sErrorMessage = oXMLCommand.ErrorMessage;
return null;
}
oCommand.Connection = oConn;
return oCommand;
}

and here is the stuff behind the XMLCommand.CreateSqlCommand method:

public SqlCommand CreateSqlCommand(string sFileName)
{
p_nErrorCode = 0;
p_sErrorMessage = "";

SerializedParameters oSP = null;
XmlSerializer oSerializer = new
XmlSerializer(typeof(SerializedParameters));

oSerializer.UnknownNode += new XmlNodeEventHandler(this.UnknownNode);
oSerializer.UnknownAttribute += new
XmlAttributeEventHandler(this.UnknownAttribute);

try
{
using (FileStream oFS = new FileStream(p_sXmlPath + sFileName,
FileMode.Open, FileAccess.Read, FileShare.Read))
{
oSP = (SerializedParameters) oSerializer.Deserialize(oFS);
oFS.Close();
}
}
catch(Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = "Error accessing or serializing XML document: " +
ex.Message;
goto Failed;
}
oSerializer = null;
Failed:
if (ErrorCode > 0) return null;
return oSP.CreateSqlCommand();
}

so is there anything in here that looks like it would likely cause a
memory leak? many thanks for the scrutiny and suggestions.

jason

[Peter Bromberg [C# MVP]]

Couple things that stood right out for me (others may have more comments):

First, it appears that the using statement near the top for your SqlConnection
doesn't have anything "inside it". The whole idea of this statement is to
automatically call Dispose on the object after the closing brace. So for
example:

using (SqlConnection oConn = new SqlConnection(sConnection))
{
// your business logic here
}

The other pattern I see is that you have a bunch of try statments with no
closing Finally statement in which one would normally close a connection.
Basicaly if you are going to do
oCommand.ExecuteNonQuery();

you want to have a finally block that will close the connection on an
exception since there isn't likely anything further down the line that you
could ever do.

I'd refactor the whole thing, and get rid of the "GOTO's".

Good Luck,
peter
--
Co-founder, Eggheadcafe.com developer portal:
http://www.eggheadcafe.com
UnBlog:
http://petesbloggerama.blogspot.com


"jason" wrote:

my w3wp.exe process has recently gotten an ever-expanding memory
footprint. the two most likely causes are either a memory leak in the
object library it uses, or session variables not getting cleaned up
correctly. this post is to check one thing in the object library, to
make sure that the "using" statement is being used correctly.

yes, i know that the use of error messaging is inadvisable and
unnecessary in .NET languages. we do plan to migrate this to use
exceptions in the future, however right now 90% of the consumers of
this object library are non.NET applications which cannot handle the
..NET exception classes.

public override bool LoadByID(string sID, string sConnection)
{
p_nErrorCode = 0;
p_sErrorMessage = "";

if (AuthorityID = Guid.Empty) return false;

bool bReturn = true;

using (SqlConnection oConn = new SqlConnection(sConnection))
using (SqlCommand oCommand =
this.CreateSqlCommand("parameterlist.xml", oConn))
{
try { oConn.Open(); }
catch (Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = "Error opening DB conenction during .DBLoadByID:
" + ex.Message;
goto Failed;
}
oCommand.Parameters["@objectid"].Value = new Guid(sID);
oCommand.Parameters["@authorityid"].Value = p_gAuthorityID;

try { oCommand.ExecuteNonQuery(); }
catch (Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = "Sql exception during LoadByID: " + ex.Message;
goto Failed;
}
if (Convert.IsDBNull(oCommand.Parameters["@rowcount"].Value))
{
p_nErrorCode = 10;
p_sErrorMessage = "Null rowcount during LoadByID.";
goto Failed;
}

int nEffect =
Convert.ToInt32(oCommand.Parameters["@rowcount"].Value);
if (nEffect == 1)
{
p_gID = new Guid(sID);
p_dtDateCreated =
GetDateTimeParameter(oCommand.Parameters["@datecreated"]);
p_dtDateModified =
GetDateTimeParameter(oCommand.Parameters["@datemodified"]);
p_gModifiedBy = (Guid)oCommand.Parameters["@modifiedby"].Value;

p_bLoaded = true;
}
else
{
p_nErrorCode = 10;
p_sErrorMessage = "Unexpected rowcount (" + nEffect + ") during
LoadByID.";
goto Failed;
}
Failed:
if (ErrorCode > 0) bReturn = false;
}
return bReturn;
}
and here is the stuff behind the function call above
"CreateSqlCommand":

public SqlCommand CreateSqlCommand(string sFileName, SqlConnection
oConn)
{
XMLCommand oXMLCommand = new XMLCommand();
SqlCommand oCommand = oXMLCommand.CreateSqlCommand(sFileName);
if (oCommand == null)
{
p_nErrorCode = oXMLCommand.ErrorCode;
p_sErrorMessage = oXMLCommand.ErrorMessage;
return null;
}
oCommand.Connection = oConn;
return oCommand;
}

and here is the stuff behind the XMLCommand.CreateSqlCommand method:

public SqlCommand CreateSqlCommand(string sFileName)
{
p_nErrorCode = 0;
p_sErrorMessage = "";

SerializedParameters oSP = null;
XmlSerializer oSerializer = new
XmlSerializer(typeof(SerializedParameters));

oSerializer.UnknownNode += new XmlNodeEventHandler(this.UnknownNode);
oSerializer.UnknownAttribute += new
XmlAttributeEventHandler(this.UnknownAttribute);

try
{
using (FileStream oFS = new FileStream(p_sXmlPath + sFileName,
FileMode.Open, FileAccess.Read, FileShare.Read))
{
oSP = (SerializedParameters) oSerializer.Deserialize(oFS);
oFS.Close();
}
}
catch(Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = "Error accessing or serializing XML document: " +
ex.Message;
goto Failed;
}
oSerializer = null;
Failed:
if (ErrorCode > 0) return null;
return oSP.CreateSqlCommand();
}

so is there anything in here that looks like it would likely cause a
memory leak? many thanks for the scrutiny and suggestions.

jason

[jason]

> First, it appears that the using statement near the top for your SqlConnection

doesn't have anything "inside it".
if i'm not mistaken, that syntax is identical to nesting using
statements.

using (Object obj1)
using (Object obj2)
{
...
}

is the same thing as saying

using (Object obj1)
{
using (Object obj2)
{
...
}
}
The other pattern I see is that you have a bunch of try statments with no
closing Finally statement in which one would normally close a connection.
ahh, that does sound like a problem. so the garbage collection built
into the using statement won't clean up an unclosed connection? i would
need to close the connection inside the catch blocks, THEN the using
statement should clean up the connections?

that does sound like a very likely culprit, i will give it a shot.
I'd refactor the whole thing, and get rid of the "GOTO's".

oh believe me, we can't wait to do that. as soon as we can convince
management to let us do a .NET conversion of the consuming
applications, we can throw exceptions like a sane object library.

thanks,

jason

[Marcus Andrén]

On 30 Nov 2005 07:55:20 -0800, "jason" <ia****@yahoo.com> wrote:

my w3wp.exe process has recently gotten an ever-expanding memory
footprint. the two most likely causes are either a memory leak in the
object library it uses, or session variables not getting cleaned up
correctly. this post is to check one thing in the object library, to
make sure that the "using" statement is being used correctly.
You seem to be using the using statement correctly. Using will always
make sure that the object is disposed when the leaving the using
statement scope.

Note: Disposing of the SqlConnection will automatically cause the
SqlCommand to be disposed so you don't have to call disposable on
both, only on the container (SqlConnection).

yes, i know that the use of error messaging is inadvisable and
unnecessary in .NET languages. we do plan to migrate this to use
exceptions in the future, however right now 90% of the consumers of
this object library are non.NET applications which cannot handle the
.NET exception classes.

Given the constraints that you have to use errorcodes/strings, I would
program it in the following way.

First, I wouldn't use multiple try-catch statements to create specific
error messages. It makes the code much less readably. It is in most
cases better to just rely on the exception message.

Second, while using is good syntactic sugar and does dispose of the
connections, in this case due to the need to handle error codes, I
would recommend using the more generic try-catch-finally.

The following code is a good way to program it. It simplifies program
flow and removes the need for goto by taking advantage of of
exceptions.

public override bool LoadByID(string sID, string sConnection)
{
// This is weakest point of the method that I could see.
// You are returning false, but not setting an error code or error.
// Why is Guid.Empty handled differently from all other exceptions?
if (AuthorityID = Guid.Empty) return false;

// Needs to be outside since it is needed in finally
SqlConnection oConn = null;
try
{
oConn = new SqlConnection(sConnection);
SqlCommand oCommand = this.CreateSqlCommand("parameterlist.xml",
oConn);

oConn.Open();
oCommand.Parameters["@objectid"].Value = new Guid(sID);
oCommand.Parameters["@authorityid"].Value = p_gAuthorityID;
oCommand.ExecuteNonQuery();

if (Convert.IsDBNull(oCommand.Parameters["@rowcount"].Value))
throw new ApplicationException("Null rowcount during
LoadByID.");

int nEffect =
Convert.ToInt32(oCommand.Parameters["@rowcount"].Value);

if (nEffect != 1)
throw new ApplicationException("Unexpected rowcount (" + nEffect
+ ") during LoadByID.");
p_gID = new Guid(sID);
p_dtDateCreated =
GetDateTimeParameter(oCommand.Parameters["@datecreated"]);
p_dtDateModified =
GetDateTimeParameter(oCommand.Parameters["@datemodified"]);
p_gModifiedBy = (Guid)oCommand.Parameters["@modifiedby"].Value;
p_bLoaded = true;

//Ugly, but required since you aren't allowed to throw exceptions
p_nErrorCode = 0;
p_sErrorMessage = "";
return true;
}
catch (Exception ex)
{
p_nErrorCode = 10;
p_sErrorMessage = ex.Message;
return false;
}
finally
{
//Make sure that the connection is disposed in all cases
if(oConn != null)
oConn.Dispose();
}
}

--
Marcus Andrén

[jason]

Thank you very, very much Marcus. That might be the most useful
response I've gotten on usenet.

Jason