Blank Destructor - is this code okay?

[Chris Ashley]

I have inherited some code and I am debug some connection pooling
errors. Is there anything wrong with the implementation of this class?
I am just curious about the empty destructor and implementation of
IDisposable as I have never seen this done before.

Code:



/// <summary>
/// This is the base class for openning and closing the db connection
/// </summary>
public class DataConnection : IDisposable
{
private SqlConnection sqlConn;
protected SqlCommand sqlCmd;

protected DataConnection()
{
try
{
sqlConn = new SqlConnection(GetDBaseConnectionString());
sqlConn.Open();

sqlCmd = new SqlCommand();
sqlCmd.Connection = sqlConn;
}
catch(System.Data.SqlClient.SqlException eSql)
{
throw new ArgumentException("Error opening database: " +
eSql.Message);
}
}


~DataConnection()
{
}


/// <summary>
/// Our implementation of the Dispose funtion from IDisposable.
/// </summary>
public void Dispose()
{
// Dispose of our db connection, and return it to the pool
sqlConn.Close();
sqlConn.Dispose();
GC.SuppressFinalize(sqlConn);

// Dispose our command object (not as important as the conn, but we
may as well do it here anyway)
sqlCmd.Dispose();
GC.SuppressFinalize(sqlCmd);

/* NOTE: We do NOT dispose ourself here. We have disposed of the db
connection,
which is the most important thing, so we can leave ourselves to be
cleaned up by the GC.
*/
}


private string GetDBaseConnectionString()
{
return ConfigurationSettings.AppSettings["DBConnStr"];
}
}

 

[Richard Blewett [DevelopMentor]]

I have inherited some code and I am debug some connection pooling
errors. Is there anything wrong with the implementation of this class?
I am just curious about the empty destructor and implementation of
IDisposable as I have never seen this done before.

Code:



/// <summary>
/// This is the base class for openning and closing the db connection
/// </summary>
public class DataConnection : IDisposable
{
private SqlConnection sqlConn;
protected SqlCommand sqlCmd;

protected DataConnection()
{
try
{
sqlConn = new SqlConnection(GetDBaseConnectionString());
sqlConn.Open();

sqlCmd = new SqlCommand();
sqlCmd.Connection = sqlConn;
}
catch(System.Data.SqlClient.SqlException eSql)
{
throw new ArgumentException("Error opening database: " +
eSql.Message);
}
}


~DataConnection()
{
}


/// <summary>
/// Our implementation of the Dispose funtion from IDisposable.
/// </summary>
public void Dispose()
{
// Dispose of our db connection, and return it to the pool
sqlConn.Close();
sqlConn.Dispose();
GC.SuppressFinalize(sqlConn);

// Dispose our command object (not as important as the conn, but we
may as well do it here anyway)
sqlCmd.Dispose();
GC.SuppressFinalize(sqlCmd);

/* NOTE: We do NOT dispose ourself here. We have disposed of the db
connection,
which is the most important thing, so we can leave ourselves to be
cleaned up by the GC.
*/
}


private string GetDBaseConnectionString()
{
return ConfigurationSettings.AppSettings["DBConnStr"];
}
}

Remove the destructor. If no-one calls Dispose its just going to hurt the GC
by getting the objet promoted into gen1 at the least

Regards

Richard Blewett - DevelopMentor
http://www.dotnetconsult.co.uk/weblog
http://www.dotnetconsult.co.uk?

 

[Jon Skeet [C# MVP]]

I have inherited some code and I am debug some connection pooling
errors. Is there anything wrong with the implementation of this class?
I am just curious about the empty destructor and implementation of
IDisposable as I have never seen this done before.

<snip>

Yes, there's something wrong with it - having an empty destructor is an
unnecessary performance cost, as when it is first noticed by the GC as
eligible for collection, it will wait to be finalized (which won't do
anything) and *then* be collected.

Also, the calls to SuppressFinalize are unnecessary, as the Dispose
calls on the other objects will already have done the same thing.

The comment of "we do not need to dispose of ourselves here" is rather
odd, too. It suggests that the author believed that one could manually
"free" an object, which isn't true. Quite what he would do otherwise is
unclear...

 

[Chris Ashley]

<snip>

Yes, there's something wrong with it - having an empty destructor is an
unnecessary performance cost, as when it is first noticed by the GC as
eligible for collection, it will wait to be finalized (which won't do
anything) and *then* be collected.

Also, the calls to SuppressFinalize are unnecessary, as the Dispose
calls on the other objects will already have done the same thing.

Thanks for the help, guys.

Should I not provide a destructor implementation in case something
fails to call dispose, or will the compiler take care of it if I do not
implement a destructor at all?

Removing the blank destructor seems to have taken care of my pooling
errors at least.

Regards,

Chris

 

[Jon Skeet [C# MVP]]

Should I not provide a destructor implementation in case something
fails to call dispose, or will the compiler take care of it if I do not
implement a destructor at all?

The garbage collector will collect things which aren't referenced any
more.
If things have *direct* handles to unmanaged resources, they should
have destructors to make sure those resources are released.

If things have *indirect* handles to unmanaged resources (eg a
reference to a Stream or SqlConnection) they should implement
IDisposable and call Dispose on those references. No destructor is
required, because the objects behind those references should have the
relevant destructors.

Destructors are *not* responsible for freeing managed memory in .NET.
That's the garbage collector's job.

Removing the blank destructor seems to have taken care of my pooling
errors at least.

Goodo.

Jon

 

[Jon Skeet [C# MVP]]

<"Richard Blewett [DevelopMentor]" <richard at nospam dotnetconsult

Remove the destructor. If no-one calls Dispose its just going to hurt the GC
by getting the objet promoted into gen1 at the least

The object will get promoted anyway - SuppressFinalize isn't called on
the object itself, only on the SQL connection and command...