Why does Code Analysis fire CA2000 on this method?

Question

Visual Studio Code Analysis generates the warning "Dispose objects before losing scope" (CA2000) on the monitor variable in this method.

private void MonitorJob(IJob job, CancellationToken cancellationToken)
{
    var monitor = new JobMonitor(job, _backend);  // <- CA2000
    try
    {
        var task = monitor.Run(cancellationToken);
        _activeJobs[task] = monitor;
    }
    catch
    {
        monitor.Dispose();
        throw;
    }
}

I understand what CA2000 does, and I'm usually able to work out why my code violates the rule and make the appropriate changes.

In this case, however, I'm stumped - is this really a false positive, or am I missing something?

Using Visual Studio 2015 Enterprise Edition, targeting .NET 4.5, using C# 6.

It is not obvious to a tool that the Run() method will *always* throw an exception. Or that it might be disposed later because you store it in _activeJobs, assuming you do. Just suppress the warning. — Hans Passant, Apr 27 '16 at 05:07
@zerkms `using(var monitor = new JobMonitor()) { var task ... }`. monitor will be disposed and exception will be thrown. — qxg, Apr 27 '16 at 05:36
@qxg any chance you have noticed this `_activeJobs[task] = monitor;` line? — zerkms, Apr 27 '16 at 05:36
Replacing the *catch* with a *finally* would mean the monitors sorted in `_activeJobs` were inactive; not the required behaviour. — Bevan, Apr 27 '16 at 05:44
Your code will dispose off the resource only in case of an Exception. What about normal flow. Therefore, it is done in a `finally` block. But that does not mean to remove the `catch` block. So the ideal flow is `try` for some action, `catch` if something goes wrong, `finally` react as if nothing happened - in the sense no resource was ever requested :D — Ravi Tiwari, Apr 27 '16 at 06:45
@RaviTiwari Normal flow is to create the monitor and trigger it running asynchronously. Both the `Task` and the `JobMonitor` are then stored in the dictionary `_activeJobs` for use elsewhere. The monitor **must not** be disposed in this case - I added the catch to ensure it's cleaned up if `Run()` throws, but only in that case. — Bevan, Apr 27 '16 at 10:06

score 2 · Accepted Answer · answered Apr 27 '16 at 06:38

You could leak this disposable if an exception is thrown here:

private void MonitorJob(IJob job, CancellationToken cancellationToken)
{
    var monitor = new JobMonitor(job, _backend);

    // <- Exception

    try
    {
        var task = monitor.Run(cancellationToken);
        _activeJobs[task] = monitor;
    }
    catch
    {
        monitor.Dispose();
        throw;
    }
}

This could be caused by say a ThreadAbortException or any other exception that's injected into a thread by the runtime. I'd suggest declaring the variable outside of the try block but assign it within. Also, set it to null on successfully assigning it to _activeJobs.

private void MonitorJob(IJob job, CancellationToken cancellationToken)
{
    JobMonitor monitor;

    try
    {
        monitor = new JobMonitor(job, _backend);
        var task = monitor.Run(cancellationToken);
        _activeJobs[task] = monitor;
        monitor = null;
    }
    finally
    {
        if(monitor!=null)
        {
            monitor.Dispose();
        }
        throw;
    }
}

Even then though, it may not be enough to shut up the warning, at which point I'd suggest adding a suppression for it.

This all makes sense, but leaves me with a (minor) followup question: Why doesn't *other* code in the same project that also follows the pattern `var x = new Y(); try..catch` also violate CA2000? — Bevan, Apr 27 '16 at 18:41
For example, the accepted answer here http://stackoverflow.com/a/9663477/30280 has exactly this structure in suggested "safe" version of `CreateFirstObject()`. — Bevan, Apr 27 '16 at 18:44

qxg · Answer 2 · 2016-04-27T08:58:32.310

I assume that _activeJobs[task] = monitor; is a simple assignment and doesn't throw exception. If so separate storing monitor from creating monitor.

private void MonitorJob(IJob job, CancellationToken cancellationToken)
{
    Task task;
    var monitor = CreateJobMonitor(job, _backend, out task);
    _activeJobs[task] = monitor;
} 

private JobMonitor CreateJobMonitor(IJob job, CancellationToken cancellationToken, out Task task)
{
    var monitor = new JobMonitor(job, _backend);
    try
    {
        task = monitor.Run(cancellationToken);
        return monitor;
    }
    catch
    {
        monitor.Dispose();
        throw;
    }

This way, CreateJobMonitor means either returning a valid object or throwing exception. There is no chance to return disposed object reference.

Why does Code Analysis fire CA2000 on this method?

2 Answers2