Why does this method cause Code Analysis error CA2000: Call Dispose()

Question

I'm building my project with the "Microsoft Minimal Rules" code analysis set and it gives me CA2000 on this method:

private Timer InitializeTimer(double intervalInSeconds)
{
    Timer timer = null;

    try
    {
        timer = new Timer { Interval = intervalInSeconds * 1000, Enabled = true };
        timer.Elapsed += timer_Elapsed;
        timer.Start();
    }
    catch
    {
         if (timer != null)
         {
             timer.Dispose();
         }
    }
    return timer;
}

This method just creates a new System.Timers.Timer from an interval in seconds. I have three such timers running (one for every second, one every minute and one every half hour). Maybe it's better to have one timer and check in the elapsed event handler whether a minute or a half hour has passed, but I don't know, this is easier at this time, it's inherited code and I don't want to break everything yet.

This method gives me the infamous

Warning 21  CA2000 : Microsoft.Reliability : In method 'TimerManager.InitializeTimer(double)', call System.IDisposable.Dispose on object '<>g__initLocal0' before all references to it are out of scope.

Now I AM calling Dispose in the catch and thought this would be enough? I'm also disposing all timers in the class' own IDisposable implementation.

What am I missing here?

Take out the object initializer syntax - that's what's triggering the warning (since the object exists, constructed, but not assigned to `timer` while the initializer runs. I'm trying to find the dupe question that deals with this. — Damien_The_Unbeliever, May 22 '12 at 07:32
possible duplicate of [Object initializers in using-block generates code analysis warning CA2000](http://stackoverflow.com/questions/3514902/object-initializers-in-using-block-generates-code-analysis-warning-ca2000) — Damien_The_Unbeliever, May 22 '12 at 07:32
Well, don't throw the initializer syntax away for a dumb tool. The warning is bogus, there's nothing to dispose until Start() is called. Swallowing exceptions and returning a disposed object, now *that* would be something to complain about. — Hans Passant, May 22 '12 at 07:39
@HansPassant - Since [`Interval`](http://msdn.microsoft.com/en-us/library/system.timers.timer.interval) can throw an `ArgumentException`, the analysis is correct that this disposable object will be leaked without having `Dispose` called. There's nothing documented (that I can find) to say that you don't need to `Dispose` the timer if `Start` hasn't been called (although this may be true of the current implementation). — Damien_The_Unbeliever, May 22 '12 at 10:21
We would not have this site if that was documented. Sometimes we have to use your own noggins. — Hans Passant, May 22 '12 at 12:05

score 2 · Answer 1 · answered May 22 '12 at 07:27

2

You only call Dispose in case of an exception (which you should never handle with a catch-all block BTW, but that is another story). In case of a no exceptions, you don't dispose the Timer object.

Either add a finally block and move the Dispose there, or use a using block.

answered May 22 '12 at 07:27

Christian.K

47,778
10
99
143

Since the Timer is returned on "non-exception", shouldn't this be okay? Or rather, why should it *always* be disposed? – May 22 '12 at 07:28
Okay, but I want the timer to survive past the method, since it's being assigned to a field in my class. I am disposing my fields in the IDisposable implementation. My try-catch was an attempt to suppress the warning. – Davio May 22 '12 at 07:29
@pst Yes, that should be OK, but CodeAnalysis may not detect it. You could suppress that particular incident then. – Christian.K May 22 '12 at 07:29

score 1 · Answer 2 · answered May 22 '12 at 07:29

1

The warning tells you that you are creating a disposable object and not disposing it in all cases. If you are properly disposing it in some other method, then you can safely suppress this warning (you can do that using the SuppressMessageAttribute).

answered May 22 '12 at 07:29

Marek Dzikiewicz

2,844
1
22
24

That's what I'm doing in other cases (Unity's LifeTimeManager), but wanted to check first before I'm doing it here. – Davio May 22 '12 at 07:31
Yeah, I get that warning quite often with long-running objects, such as timers. It's just there to make you aware that you have to dispose the object somewhere else. – Marek Dzikiewicz May 22 '12 at 07:32

score 1 · Answer 3 · answered May 22 '12 at 07:42

Okay, I edited it like this:

private Timer InitializeTimer(double intervalInSeconds)
    {
        Timer tempTimer = null;
        Timer timer;
        try
        {
            tempTimer = new Timer();
            tempTimer.Interval = intervalInSeconds * 1000;
            tempTimer.Enabled = true;
            tempTimer.Elapsed += timer_Elapsed;
            tempTimer.Start();
            timer = tempTimer;
            tempTimer = null;
        }
        finally
        {
            if (tempTimer != null)
            {
                tempTimer.Dispose();
            }
        }
        return timer;
    }

This is per the CA2000 docs and it doesn't give a warning. I had overlooked the fact that the object initializer syntax creates a temporary object which may not be disposed.

Thanks, guys!

You might want to change that finally to a catch with rethrow. Otherwise, you're going to dispose your timer before you return it from the method, even if no exception is thrown. — Nicole Calinoiu, May 23 '12 at 00:24
@NicoleCalinoiu It only disposes if the temp is not null and the temp is only not null if it didn't reach the last line of the try block, which means an exception was thrown. This just gets around having to catch and rethow an exception. — juharr, Aug 15 '13 at 18:56

score -1 · Answer 4 · edited May 23 '17 at 12:08

I think it is much better to use "using" instead of "try/finally" reference

private Timer InitializeTimer(double intervalInSeconds)
{
    Timer timer;
    using (var tempTimer = new Timer())
    {
        tempTimer.Interval = intervalInSeconds * 1000;
        tempTimer.Enabled = true;
        tempTimer.Elapsed += timer_Elapsed;
        tempTimer.Start();
        timer = tempTimer;
    }
    return timer;
}

Why does this method cause Code Analysis error CA2000: Call Dispose()

4 Answers4

Linked