Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mhamri opened this issue Jun 12, 2017 · 8 comments

Comments

@mhamri
Copy link

mhamri commented Jun 12, 2017

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;
        }
@mhamri mhamri changed the title Method that are returning disposable object shouldn't marked Method that are returning disposable object, the return object shouldn't marked inside the method Jun 12, 2017
@BADF00D
Copy link
Owner

BADF00D commented Jun 12, 2017

@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
Copy link
Author

mhamri commented Jun 12, 2017

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
Copy link
Owner

BADF00D commented Jun 12, 2017

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
Copy link
Author

mhamri commented Jun 12, 2017

thanks, yes, it's the full name

@BADF00D BADF00D self-assigned this Jun 15, 2017
BADF00D pushed a commit that referenced this issue Jun 15, 2017
@BADF00D
Copy link
Owner

BADF00D commented Jun 15, 2017

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

@mhamri
Copy link
Author

mhamri commented Jun 28, 2017

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

@BADF00D BADF00D added this to the Release 1.0.0 milestone Jun 28, 2017
@BADF00D
Copy link
Owner

BADF00D commented Jul 7, 2017

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 added a commit that referenced this issue Sep 28, 2017
BADF00D added a commit that referenced this issue Sep 28, 2017
@BADF00D
Copy link
Owner

BADF00D commented Sep 28, 2017

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.

@BADF00D BADF00D closed this as completed Sep 28, 2017
@BADF00D BADF00D added bug and removed await reply labels Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants