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

Suggestion: Allow code marking to be disabled/less intrusive #112

Open szalkerous opened 5 years ago

szalkerous commented 5 years ago

More often than not I find DisposableFixer is marking the code constantly for objects that are disposed elsewhere in the code properly. This results in the code editor being littered with code marking that is unnecessary (see example 1). I realize it would be improbable to be able to efficiently detect every object's proper disposal, so a good compromise would be the configurable setting to disable code marking (using the error list or build process as a reference instead) or use the CopFx style marking (small dots under the beginning of the object, as seen in example 2)

Thank you for considering this suggestion, it would make a world of difference!

example1

example2

BADF00D commented 5 years ago

Hi, thanks for the suggestion. You are using this with SharpDx, cool.

I have a few questions:

  1. Is the _SwapChain instance a field, that is disposed when dxgiFactory gets disposed? Or where does it get disposed?
  2. Can you add a screenshot for the CopFx style, you mentioned?
  3. Which version of SharpDx are you using? In the current source I can't find the correct source (SwapChain1).
szalkerous commented 5 years ago
You are using this with SharpDx, cool.

To be honest, I use DisposableFixer as a VS-wide extension for all my projects. However for SharpDX it is even more valuable as failure to properly dispose causes system-level DX warnings to be thrown, and significant memory leaks. Very undesirable.

1. Is the _SwapChain instance a field, that is disposed when dxgiFactory gets disposed? 
Or where does it get disposed?

All sharpDX components have to follow the DirectX model of destruction; meaning, they have to deallocate their resources in the reverse order in which they were created (a good example article about this subject).

With this in mind, the screenshot I provided you was a sample of a function called "InitDX" which does all the initial allocation and configuration of each required object in order, and stores them in the class as private objects (which are dually exposed as public Fields for ease of use in certain cases).

When the class is Disposed, it calls a function "DestroyDX" which disposes and destroys each object:

Util.DisposeSafe(ref _Context);
Util.DisposeSafe(ref _BackBuffer);
Util.DisposeSafe(ref _SwapChain);
Util.DisposeSafe(ref _Device);
Util.DisposeSafe(ref _3DDevice);
Util.DisposeSafe(ref _Factory);

and DisposeSafe is just a simple wrapper function that does the following (I borrowed the code from the SharpDX Utilities class (Line 1186) and added my own logging functions for tracking):

 public static void DisposeSafe<T>(ref T comObject) where T : class, IDisposable
{
    if (comObject != null)
    {
        try
        {
            comObject.Dispose();
        }
        catch(Exception ex)
        {
            Logger.Core.Error("Could not properly dispose object.", ex);
    }

        try
        {
            comObject = null;
        }
        catch (Exception ex)
        {
            Logger.Core.Fatal("Could not destroy object.", ex);
        }
    }
}
2. Can you add a screenshot for the CopFx style, you mentioned?

It was included as that small second image with the red arrow.

EDIT: I did some looking into, and I think it's actually called a "code suggestion" which is new for VS2017.

The following screen shot was taken from the current-day manifestation of the CopFX project located at the Visual Studio Marketplace.

code-suggest

3. Which version of SharpDx are you using? In the current source I can't find the correct source 

This is the latest stable version, 4.2.0. SharpDX code is rather difficult to piece together as it's mostly partial classes that get combined. Even I get lost trying to find things in the source, and documentation tends to be very sparse. Most of the time, SharpDX interface objects are simply COM wrappers around the native DirectX interface (that usually matches it in name).

So for SwapChain1, it would be wrapping IDXGISwapChain1.

Thank you so much for considering this improvement!

BADF00D commented 5 years ago

Hi, we are getting closer. I think I can help you.

Severity Level: The current Severity Level is set to Error, for all Diagnostics detected by this extension. If you are using DisposableFixer as VS-Extension, then they are displayed as warnings. This is a design decision made by Microsoft. If you are using the NuGet-Package. then the diagnostics are reported as errors an they will fail your build. You can change these by your own as explained here. 2019-03-19_07-23-28

Before: image

After: image

Until I checked this, I was not sure this would change the underlining. But it works.

Special Dispose Methods:

The DisposableFixer is already aware of the situation, that a Dispose class is not always like member?.Dispose(). For instance:

  1. If you close a StreamReader, it the same as calling streamreader.Dispose().
  2. At work, we use certain base test classes, that you have to override in order to dispose you stuff. This method is considered as a Dispose method.
  3. A method marked with NUnit.Framework.TearDownAttribute is also considered as Dispose method The default configuration already contains some of these specials. Currently this is hard-coded, but I intend to make this configurable in version 2.0.0.

To add this, I need the complete signature of the dispose method including class and namespace name. As I see it, this should be namespace: SharpDX class: Utils signature: public static void Dispose(ref T comObject) where T : class, IDisposable

But this will not help you, since you are using your own wrapper. I don't think this is open source? I don't want to add configurations for closed source projects, because I can't test and verify that these are working and it would be to much.

But we have an other possibility: Can you test something for me? Can you reference NUnit3 in your project and mark your method SafeDispose with the NUnit.Framework.TearDownAttribute? Afterwards there should be no more warnings in your code, because this attribute marks you method as Dispose method. If this does not work, can you provide a complete example where you create and destroy theses resources, in the same way your are doing it in your productive code?

If this works, then I can create an annotations package, that you can reference (pretty much like JetBrains.Annotations).

BADF00D commented 5 years ago

@szalkerous Is this still an issue?

szalkerous commented 5 years ago

@szalkerous Is this still an issue?

@BADF00D I apologize for not keeping up with this issue. I will take your suggestion regarding the code marking and try it out in VS2019, along with the NUnit.Framework.TearDownAttribute test.

As for the SharpDX code, it is all open source. The utilities class with the Dispose functionality is located here.