Sql server deadlock retry

[Raymon]

I have this method in a data utility class used to retrieve data from
sql server.
It only checks for sever deadlock in which case it will retry the
operation 10 times before giving up.
Any other error is thrown back to the caller.

When I run code analysis on this it gives the warning datatable dt
is not disposed of in all exception paths.
How can I dispose of the datatable?
It is the very data that is being returned.

public static DataTable GetTable(SqlCommand cmd)
{
bool done = false;
int tries = 0;
DataTable dt = new DataTable();

while (!done)
{
try
{
using (SqlDataAdapter adp = new SqlDataAdapter(cmd))
{
adapter.Fill(dt);
}
done = true;
}
catch (SqlException e)
{
foreach (SqlError er in e.Errors)
{
if (er.Number == 1205 && tries < 10)
{
tries++;
System.Threading.Thread.Sleep(5);
break;
}
else
{
throw;
}
}
}
}
return dt;
}

 

[Arne Vajhøj]

I have this method in a data utility class used to retrieve data from
sql server.
It only checks for sever deadlock in which case it will retry the
operation 10 times before giving up.
Any other error is thrown back to the caller.

When I run code analysis on this it gives the warning datatable dt
is not disposed of in all exception paths.
How can I dispose of the datatable?
It is the very data that is being returned.

You can not Dispose and return it.

The analysis tool is broken.

Arne

 

[Arne Vajhøj]

The analysis tool is not complaining about not disposing an object that's
being returned. It's complaining about not disposing it when it's _not_
returned.

Seems to me, that's a perfectly valid warning.

If that is the case, then it is valid to give a warning and just
the warning text that should be a bit clearer.

Arne

 

[Raymon]

It's not being returned when you throw an exception.

The CA warning should go away if you dispose "dt" before the "throw"
statement.

Oddly enough, the warning message is quite specific about this. That's
exactly what it means by "in all exception paths". It's not concerned about
the non-exception code path. Just the one where you throw an exception
instead of returning the DataTable instance.

Read all compiler messages carefully. They often have exactly the
information you need.

Pete

OK, thanks.

What would be the case if I use a finally block?
If I add a finally block to dispose the object there, would the object
be returned when there is no exception?

 

[Raymon]

Code in a "finally" block will always execute, whether the method succeeds
or not. You don't want to dispose in a "finally" block, because you _do_
want to return the object on success. Your method would still return the
object, but in that case it would always be disposed.

You really do just want to dispose the object just before the "throw"
statement (or more generally, before _any_ thrown exception in the
method...it just happens that in the code you posted, there's only one
place that happens, in the "catch" block). That will ensure it's disposed
on failure, but not on success when you're able to return the reference to
the object.

Pete

One more question on using "using" block.
If I have an object, MyObject, that implements IDisposable would the
follwing code work?
That is it returns the object in case of success and dispose it in the
case of failure.


Try
[
using (MyObject o = new MyObject())
{
// do stuff here

return o;
}
}
catch
{
// handle exception
}

 

[Raymon]

No. The whole point of a "using" block is that the object
declared/specified in the "using" statement itself is guaranteed to be
disposed before control exits the block. Using the example you posted, the
object "o" would _always_ be disposed, not just when an exception occurred.

In fact, the "using" block is implemented by the compiler as a
"try/finally" block. So for the exact same reason a "try/finally" would not
be appropriate, neither would "using".

Pete

Often in my methods I return complex data in a datatable that I
construct in code.

Consider this very simple example:

private DataTable GetData()
{
DataTable dt = new DataTable();
dt.Columns.Add("id", typeof(int));

DataRow dr = dt.NewRow();
dr["id"] = 12345;
dt.Rows.Add(dr);

return dt;
}

The code analysis gives the warning that the object dt should be
disposed of.
How can it be disposed and at the same time return a valid object?

Yet when I tried the following code:

private DataTable GetData()
{
using (DataTable dt = new DataTable())
{
dt.Columns.Add("id", typeof(int));

DataRow dr = dt.NewRow();
dr["id"] = 12345;
dt.Rows.Add(dr);

return dt;
}
}

it wotked, in the sense that in the calling method the dt has correct
values. It doesn't seem disposed.

But in another example I made up my own object which implements
IDisposable.
When I try the above method, with "using", the object in the calling
method doesn't have the correct values (I reset some property in
the Dispose method).
So it seems the object has indeed been disposed.

How do you explain this?
Why would datatable work with "using", but not my custom object?

 

[Raymon]

Often in my methods I return complex data in a datatable that I
construct in code.

Consider this very simple example:

private DataTable GetData()
{
DataTable dt = new DataTable();
dt.Columns.Add("id", typeof(int));

DataRow dr = dt.NewRow();
dr["id"] = 12345;
dt.Rows.Add(dr);

return dt;
}

The code analysis gives the warning that the object dt should be
disposed of.
How can it be disposed and at the same time return a valid object?

It's hard to say without a proper concise-but-complete code example and
without the exact compiler message you are getting. However, in that
example there is nothing that ensures "dt" is disposed if an exception
occurs in any of the methods called by the "GetData()" method.

So the code analysis would be correct to object to the lack of a try/catch
in which you dispose the object. If you include a try/catch to deal with
exceptions, that should satisfy the code analysis:

private DataTable GetData()
{
DataTable dt = new DataTable();

try
{
dt.Columns.Add("id", typeof(int));

DataRow dr = dt.NewRow();
dr["id"] = 12345;
dt.Rows.Add(dr);
}
catch
{
dt.Dispose();
throw;
}


return dt;
}

I usually don't have try/catch in my sub methods.
I let the exception "bubble up" and be handled at the top of
the food chain so to speeak.
Because catch then throw seems to me to be redundant.
Maybe that is not a good practice.

But you are right. The warning message invariably includes
the phrase "not in all exception paths".

So code analysis is mostly concerned about what happens
in case of an exception.

Thank you for this informative discussion.