How do I dispose this code properly?

Question

I have a quite large project which I try to keep as clean and tidy as possible. When I run the code analyzer in Visual Studio I get a reliability error which I find quite annoying. I'd really like to learn how to work around it. Here is a simplified example of what I am doing.

Here is the warning.

Warning 1 CA2000 : Microsoft.Reliability : In method 'MyExampleClassForStackOverflow.AddFeed(string)', call System.IDisposable.Dispose on object 'new FeedClassExamle()' before all references to it are out of scope.

Here is my example code:

class MyExampleClassForStackOverflow : IDisposable
{
    public ConcurrentDictionary<string, FeedClassExamle> Feeds { get; set; }

    public void AddFeed(string id)
    {
        //The warning is coming from this code block.
        //In the full code, the feed classes collects data on a specific 
        //interval and feeds them back using events.
        //I have a bunch of them and they need to be accessible so I 
        //store them in dictionaries using keys to effeciently find them.
        Feeds.TryAdd(id, new FeedClassExamle());
        Feeds[id].Start();
    }
    public void Dispose()
    {
        foreach (var item in Feeds)
            item.Value.Dispose();
    }
}

class FeedClassExamle : IDisposable
{
    public void Start()
    {

    }
    public void Dispose()
    {

    }
}

In order to test the code, use:

using (var example = new MyExampleClassForStackOverflow())
{

}

Any suggestion would be welcome.

good question. i always suppress this warning in such situations... it would be good to know if there is any workaround — 6opuc, May 21 '12 at 02:44

score 1 · Answer 1 · answered May 20 '12 at 14:20

1

The object isn't getting Disposed of if TryAdd fails, so try doing this explicitly:

public void AddFeed(string id)
{
    FeedClassExample fce = new FeedClassExamle();
    if (!Feeds.TryAdd(id, fce))
    {
        fce.Dispose();
    } 
    Feeds[id].Start();
}

answered May 20 '12 at 14:20

Richard

29,854
11
77
120

I tried it but still get: Warning 1 CA2000 : Microsoft.Reliability : In method 'MyExampleClassForStackOverflow.AddFeed(string)', call System.IDisposable.Dispose on object 'fce' before all references to it are out of scope. – BlueVoodoo May 20 '12 at 14:24
Do you get the same message if you do `Feeds[id] = new FeedClassExamle();`? – Eugene Ryabtsev May 20 '12 at 14:32
This seems to be related to the `ConcurrentDictionary` class - the message goes away when this is replaced by a standard dictionary class. It looks like the code analysis doesn't realise that ConcurrentDictionary is keeping a reference. – Richard May 20 '12 at 14:34
@EugeneRyabtsev - Yes, same warning. – BlueVoodoo May 20 '12 at 14:35
@Richard - Ah, that's annoying. Worst case, I guess I can implement some extensions to lock a standard Dictionary to get rid of the warning. – BlueVoodoo May 20 '12 at 14:37
1

@BlueVoodoo: Well you can also suppress the warning if you're sure of what you're doing: [LINK](http://stackoverflow.com/questions/3978722/how-are-idisposable-objects-passed-to-base-constructor-arguments-handled-in-c-sh) – digEmAll May 20 '12 at 14:38
Indeed - if you need a ConcurrentDictionary, use it rather than altering your code to something you don't need just to avoid a SuppressMessage! Suppress message is fine, especially if you fill in the Justification field so you know why it was suppressed! – Richard May 20 '12 at 14:40
@digEmAll - Yes, that would be a better option. – BlueVoodoo May 20 '12 at 14:40
@Richard - Yep, unless anyone has suggestion to make the warning go away without suppressing it, I'll do just that. – BlueVoodoo May 20 '12 at 14:44

score 1 · Accepted Answer · answered May 20 '12 at 20:47

The warning exists because the code analysis tools can't determine whether the object will get disposed correctly. The way your code is written, the object will not in fact get disposed correctly, but fixing the code will likely not eliminate the warning.

Fundamentally, what needs to happen is for every the AddFeed method to ensure that something will call Dispose on every FeedClassExample instance it creates. The best approach is to avoid creating a FeedClassExample instance if one already exists in the dictonary under the present ID. Failing that, the AddFeed method should either dispose of any FeedClassExample it creates but then decides not to store in the dictionary, or else swap with the one that is in the dictionary (I'm not sure what methods ConcurrentDictionary supports to do that) and then Dispose the old one. The essential requirement is that at all times outside the actual execution of AddFeed, the dictionary will hold all instances of FeedClassExample that have been created but not destroyed.

It may be informative to add a destructor in your FeedClassExample class which does nothing except log a message. If you are calling Dispose on that class correctly, the destructor will never execute. If you fail to call Dispose, it will. Thus, if the destructor ever executes, you can know you're doing something wrong.

Yes, good idea with the destructor. I may do just that. Regarding creating a new instance if one already exists, that is covered in my main code. Just missing in the simplified example here which I just used to demonstrate how to reproduce the warning. — BlueVoodoo, May 21 '12 at 12:34

Guffa · Answer 3 · 2012-05-20T14:47:56.283

0

Only create the instance if it needs to be added:

if (!Feeds.ContainsKey(id)) {
  Feeds.GetOrAdd(id, new FeedClassExamle());
}

edited May 20 '12 at 14:47

answered May 20 '12 at 14:36

Guffa

687,336
108
737
1,005

Yes, this is just example code to reproduce the warning. My main code returns before adding if the key exists. – BlueVoodoo May 20 '12 at 14:42
@digEmAll: Right... Use the `GetOrAdd` method. – Guffa May 20 '12 at 14:48

How do I dispose this code properly?

3 Answers3