CODEWITHSUNDEEP
c#visual-studiodisposeusing
Turp
Turp

Reputation: 708

Disposing Objects Multiple Times

I am a developer that is maintaining and constantly making an application faster and better. I came across a piece of code that uses .Dispose(), which is inside a using statement.

Here is the code:

using (IDbCommand cmd = proxy.Connection.CreateCommand())
{
   cmd.CommandText = "usp_GetPluginInfo";
   cmd.CommandType = CommandType.StoredProcedure;
   cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@dealershipId", dealershipId));
   cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@pluginId", pluginId));
   PluginInfo[] pluginInfos = PopulatePluginsFromCommand(cmd);
   result = pluginInfos.First();
   cmd.Dispose();
}

Wouldn't the last } Dispose when the Command is finished? I didn't think that it was necessary to use Dispose() in this case.

Thanks for all of your help!

Upvotes: 3

Views: 170

Answers (7)

dcastro
dcastro

Reputation: 68760

You're right, it's not necessary. A using statement is translated by the compiler to this:

IDbCommand cmd = proxy.Connection.CreateCommand();

try
{    
    //...
}
finally
{
    if(cmd != null)
        ((IDisposable)cmd).Dispose();
}

Having said that, calling Dispose more than once should not be "hurtful" either, as a proper IDisposable implementation is supposed to be idempotent. It is, however, redundant, and you should remove it, since you're cleaning up the code.

Upvotes: 6

spidergeuse
spidergeuse

Reputation: 3

The cmd.Dispose(); is unnecessary. In a case where Dispose() is not properly implemented or made to throw the System.ObjectDisposedException, this could be an issue

Refer to the code analysis rule below The call to CA2202: Do not dispose objects multiple times

Upvotes: 0

Brian Rasmussen
Brian Rasmussen

Reputation: 116501

It's redundant and the explicit call to Dispose is not guaranteed to execute. If you want to call Dispose explicitly make sure to do it in a finally block.

Upvotes: 0

It'sNotALie.
It'sNotALie.

Reputation: 22814

There is one reason he could've put it there explicitly. If the IDbCommand isn't the one from System.Data, and the interface that was made for some reason inherited from IDisposable but used new instead of override, they could have different behaviours. But it's 99.999999999% of the time absolutely redundant.

Upvotes: 0

Jesse C. Slicer
Jesse C. Slicer

Reputation: 20157

You are correct! The using block is syntactic sugar for the following:

IDbCommand cmd = proxy.Connection.CreateCommand();
try
{
   cmd.CommandType = CommandType.StoredProcedure;
   cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@dealershipId", dealershipId));
   cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@pluginId", pluginId));
   PluginInfo[] pluginInfos = PopulatePluginsFromCommand(cmd);
   result = pluginInfos.First();}
finally
{
    if(cmd != null)
    {
        ((IDisposable)cmd).Dispose();
    }
}

So yes, the extra cmd.Dispose(); is unnecessary. In most cases, a well-written class will do nothing the second .Dispose() (using the so-called Disposable Pattern), but sometimes it will cause issues. Get rid of it!

Upvotes: 1

Markus
Markus

Reputation: 22511

using calls Dispose upon exit of the block, no matter whether the block is run successfully or an exception occurs. In your example, the explicit call to Dispose is redundant.

Upvotes: 0

hmnzr
hmnzr

Reputation: 1430

using statement always calls IDisposable.Dispose() method. cmb.Dispose() code is redundant

Upvotes: 0

Related Questions