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

Method that are returning disposable object, the return object shouldn't marked inside the method #38

Closed mhamri closed 7 years ago

mhamri commented 7 years ago

this is a method that i have, using asp.net core 1.2

method is returning a disposable object that shouldn't be marked as a warning inside the method. moreover it's base asp.net core class. (this one and ILogger, for both i get this wiggly marker under them)


public static IServiceProvider CompileAndUseDefaultServices(this IServiceCollection serviceCollection, IConfiguration configuration)
        {
            var serviceProvider = serviceCollection
                .BuildServiceProvider();

           //following lines marked as an error. but i'm returning the service provider in this function. BTW it's asp.net core base class. can't dispose it 
            serviceProvider.GetService<ILoggerFactory>()
                .AddConsole(configuration.GetSection("Logging:Console"))
                .AddProvider(new LoggerFileProvider(configuration.GetSection("Logging:File")));

            return serviceProvider;
        }
BADF00D commented 7 years ago

@mhamri Thanks for reporting the bug.

What is the return type of

serviceProvider.GetService<ILoggerFactory>()
     .AddConsole(configuration.GetSection("Logging:Console"))
     .AddProvider(new LoggerFileProvider(configuration.GetSection("Logging:File")));

Is this ILoggerFactory and does it implement IDisposable?

mhamri commented 7 years ago

yes return type is iLoggerFactory

this is the ILoggerFactory:

    //
    // Summary:
    //     Represents a type used to configure the logging system and create instances of
    //     Microsoft.Extensions.Logging.ILogger from the registered Microsoft.Extensions.Logging.ILoggerProviders.
    public interface ILoggerFactory : IDisposable
    {
        //
        // Summary:
        //     Adds an Microsoft.Extensions.Logging.ILoggerProvider to the logging system.
        //
        // Parameters:
        //   provider:
        //     The Microsoft.Extensions.Logging.ILoggerProvider.
        void AddProvider(ILoggerProvider provider);
        //
        // Summary:
        //     Creates a new Microsoft.Extensions.Logging.ILogger instance.
        //
        // Parameters:
        //   categoryName:
        //     The category name for messages produced by the logger.
        //
        // Returns:
        //     The Microsoft.Extensions.Logging.ILogger.
        ILogger CreateLogger(string categoryName);
    }
BADF00D commented 7 years ago

It seems to me, that this is one in a few cases, that should not be disposed. Unfortunately I was not able to figure out a way, to automatically detect this situations. Currently I use a list of types, that should be ignored during disposable detection. From DefaultConfiguration:

IgnoredInterfaces = new HashSet<string> {
    "System.Collections.Generic.IEnumerator"
};
IgnoredTypes = new HashSet<string> {
    "System.Threading.Tasks.Task",
};

This list is currently hard-coded, but there is a plan to make this configurable in ticket #15. For a short-term solution, I can add this interface to the hard-coded items. Is the full name Microsoft.Extensions.Logging.ILoggerFactory?

mhamri commented 7 years ago

thanks, yes, it's the full name

BADF00D commented 7 years ago

I just uploaded v0.21. can you Verify the fix?

mhamri commented 7 years ago

hi, I tried on vs2015 and v2017, both looks like has the problem. still, i'm getting the warning for ILoggerFactory.

BADF00D commented 7 years ago

I'm still not able to reproduce the problem. But I stumbled upon the original source code, so maybe this helps to reproduce the problem.

At the moment, the only difference I can see is, that the namespace from ILoggerFactory does not match its folder structure. But this should not matter at all.

BADF00D commented 7 years ago

Finally I was able to reproduce the problem. There was actually a bug that causes the problem. I added a few test, so the problem should not occur any more.

Will be part of release 0.33.