How can I dispose dataset when passed as argument

Question

I am getting code analysis error for not disposing dataset. This arises when I passed DataSet as argument to method Test2. when I am commenting call to Test2(), this error got remove. So please help me out to resolve this error.

static DataSet TestMethod(DataSet s)
        {
            try
            {

                    try
                    {
                        if (s != null)
                        {
                            s = new DataSet();
                            //some other stuff....

                        }
                        **s = Test2(s);**  //This line causes Dispose code analysis error


                    }
                    catch (InvalidCastException)
                    {
                       throw;

                    }

                return s; 
            }
            catch (Exception)
            {
               throw;
            }

            finally
            {
                if (s!=null)
                {
                    s.Dispose();
                }

            }

        }

        static DataSet Test2(DataSet s)
        {
            try
            {
                //some stuff
            }
            catch (InvalidCastException)
            {


            }
            finally
            {
                s.Dispose();
            }
            return s;
        }

Actually, the error is a symptom of seriously borked code. Why do you dispose a resource you are returning in Test2? Why do you return your input parameter anyway? Why are you overwriting an input parameter variable in your first method? You should refactor that code to be less error prone and more obvious to fellow programmers (because that includes yourself 2 months down the road). — nvoigt, Aug 22 '14 at 07:52
There's nothing in that code that says "`TestMethod` is assuming ownership of the lifetime of this object" - in fact, since it **returns** it, quite the opposite; frankly, I think it is simply an **error** to dispose anything here (not that it actually matters, for `DataSet`) — Marc Gravell, Aug 22 '14 at 08:04
btw; `catch` just to `throw` is: entirely pointless. Remove that. — Marc Gravell, Aug 22 '14 at 08:04

score 2 · Accepted Answer · edited May 23 '17 at 12:21

2

In general you can ignore an error that indicates that you should dispose the DataSet. A DataSet implements IDisposable (like also DataTable) but it doesn't hold any unmanaged resources: Should I Dispose() DataSet and DataTable?

However, why do you pass the DataSet as argument to the method at all? And why do you call Test2 with this DataSet as argument? And why do you dispose it there before you try to return it from that method?

To be honest, the code is completely broken and pointless.

Normally you need one method like following:

static DataSet TestMethod()
{
    DataSet ds = new DataSet();

    using(var con = new SqlConnection("Connection-String"))
    using(var da = new SqlDataAdapter("SELECT t.* FROM TableName t ORDER BY t.Column", con))
        da.Fill(ds);

    return ds;
}

edited May 23 '17 at 12:21

Community

1
1

answered Aug 22 '14 at 07:56

Tim Schmelter

450,073
74
686
939

Good point about the uselessness of disposing a `DataSet`. Any idea why they even let `DataSet` implement `IDisposable`? – Patrick Hofman Aug 22 '14 at 08:00
2

`IDisposable` comes from the inherited `MarshalByValueComponent` which enables visual drag&drop capabilities in visual studio. Right now, there are no unmanaged resources in those objects that would need to be cleaned up via Dispose. In theory, in future versions of the framework there may be. I strongly doubt that this ever wil be the case. – Tim Schmelter Aug 22 '14 at 08:06

score 1 · Answer 2 · answered Aug 22 '14 at 07:59

That warning isn't the worst. You dispose your DataSet in your Test2 method. Why do you do that? It will make the DataSet useless. Even returning it is useless, especially when it is the same instance as put in.

Why not let Test2 create and return a new DataSet instead of pointlessly moving things around?

score 1 · Answer 3 · answered Aug 22 '14 at 08:12

You shouldn't be disposing things you are returning, and in most cases, you shouldn't dispose things that somebody passes to you to look at (the only times you would dispose it is if the method contract made it clear that this method was responsible for the cleanup). Frankly, I don't think there's reason to dispose at all in the two methods shown. A more correct implementation would be simply:

static DataSet TestMethod(DataSet s)
{

    if (s == null) // I'm assuming this was a typo, otherwise the
                   // input is completely ignored
    {
        s = new DataSet();
        //some other stuff....
    }
    Test2(s); // note you didn't handle the return value, so it
              // can't be important or relevent
    return s; // in case we just created it
}

static void Test2(DataSet s)
{
    //some stuff
}

You'll notice a complete lack of disposal; in the scenario that somebody passes in an existing DataSet, they still want it to work. In the scenario that somebody passes in null, well - since we're handing the new one back to them as a result, it is still meaningless to dispose it.

If the caller wants the objects disposed (not that it matters in this case), that is their job:

using(var ds = ...)
{
    // ...
    TestMethod(ds);
    // ...
}

or perhaps the slightly more exotic construction scenario:

using(var rs = TestMethod(null))
{
    // ...
}

How can I dispose dataset when passed as argument

3 Answers3