search

BADF00D / DisposableFixer

This is a Visual Studio Extension and NuGet package that should identify and fix problems as memleaks while using IDisposables.
Other
35 stars 7 forks source link

Incorrect DF0021: request to dispose of a static variable #116

Closed AartBluestoke closed 5 years ago

AartBluestoke commented 5 years ago

Prerequisites

Description

When you include a static initialized disposable an inappropriate suggestion to have the class dispose of its static members is made.

Source Code


    class someTest
    {
        static IDisposable f()=>null;
        static IDisposable disposable = f(); // why does this make someTest IDisposable, disposing of someTest won't dispose of the static members, or it will invalidate the static state of the other classes?
    }

Screenshot

image

AartBluestoke commented 5 years ago

the static IDisposable f()=>null; is just a dummy payload to trigger the inspection; it could be anything (it was new SemaphoreSlim(3) in my real code)

BADF00D commented 5 years ago

Hi, thanks for reporting. This is obviously a bug. I will fix that. Btw, I wouldn't recommend to use IDisposables at static members at all, because for me its not clear who is in charge to dispose them.

BADF00D commented 5 years ago

I analyzed to problem and its a little bit more complicated then I expected initially. DF0021 stands for a field that can be disposed in dispose method. But a static field cannot be disposed in a dispose method. So this warning will get another ID and therefore introduces a breaking change. This is no big deal, but I need some time for this

AartBluestoke commented 5 years ago

in my case the lifetime of the static disposable was the lifetime of the application, guarding concurrent calls to an API; anyone using any of the endpoints on that URL needed to limit their concurrency; i was happy for the object to be 'disposed' of at final application teardown, in the same way as it was initialized at startup.

Also, thanks for your rapid response to my comments. WHen you release a new version, i'll update the analyzer in my solution and report any other "interesting" warnings (there were a few, but these were the main ones present in my code), and most of the others might be my fault :D

BADF00D commented 5 years ago

There are a few new diagnostic id for this problem. Diagnostic ids for static member are now different from none static members. The result is the codefix only pop up for none static members. In my view, static members should not be IDisposable at all, but I accept that some code need this code. If you don't like these diagnostics in your code, you can disable them via standard Roslyn features. The Ids are

These ids will be available with release 2.0.0.

AartBluestoke commented 5 years ago

In my view, static members should not be IDisposable at all

If all IDisposable held things that must be correctly disposed (eg streams that need flushing, OS objects that require explicitly releasing) then i would agree, except that many Disposables don't hold those things. OS level resources will be released as the app domain is torn down, and my specific example (SemaphoreSlim) simply closes an underlying SafeWaitHandle, I believe any disposable that is simply managing locking based on safeWaitHandle is safe to discard.

Thread is another disposable thing that wraps a SafeWaitHandle that is regularly not disposed (are you going to warn on every await?)

(re: Task : https://devblogs.microsoft.com/pfxteam/do-i-need-to-dispose-of-tasks/ )