AddSingleton with disposable instance behaviour

mark-r-cullen commented 4 years ago

Is your question related to a specific version? If so, please specify:

v2 - 2.0.13351.0

What language does your question apply to? (e.g. C#, JavaScript, Java, All)

C#

Question

Trying to use function DI to register IDocumentClient (cosmos) as a Singleton. DocumentClient is disposable.

The documentation at https://docs.microsoft.com/en-us/azure/azure-functions/functions-dotnet-dependency-injection suggests that "Singleton: The singleton service lifetime matches the host lifetime and is reused across function executions on that instance. Singleton lifetime services are recommended for connections and clients, for example SqlConnection or HttpClient instances."

However it seems like the singleton is being disposed after the first request, and on subsequent requests getting an ObjectDisposedException:

System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'DocumentClient'.
   at Microsoft.Azure.Documents.Client.DocumentClient.ThrowIfDisposed()
   at Microsoft.Azure.Documents.Client.DocumentClient.EnsureValidClientAsync()
   at Microsoft.Azure.Documents.Client.DocumentClient.GetCollectionCacheAsync()

It feels like something is not quite right here. Is this the correct behaviour for a disposable singleton or a bug? I've had a quick search and found issues with scoped being disposed incorrectly, but haven't found anything regarding singleton.

Cheers

soninaren commented 4 years ago

@mark-r-cullen Can you share the piece of code where you are trying to inject Document Client. Also have you tried using cosmos db binding? It manages the connection for you so you don't have to worry about documentClient lifetime.

mark-r-cullen commented 4 years ago

I can't really share the code, sorry. I have been trying to reproduce it in a small snippet that I can post here, but it seems like it is only actually affecting DocumentClient, which is odd.

If I create a small disposable class that checks whether it is disposed and throws an exception on a method call, which is registered as a singleton, it appears to behave as expected. If I try and use DocumentClient in the same way it seems to be getting disposed.

Once I have got a snippet of code that can reproduce what I am seeing (or figure out whats wrong) I will post it up here.

    public static class MySingletons
    {
        private static readonly Lazy<IDocumentClient> LazyDocumentClient = new Lazy<IDocumentClient>(() =>
                                                                                                     {
                                                                                                         var appSettings = new AppSettings();

                                                                                                         return new DocumentClient(appSettings.CosmosDBEndpoint,
                                                                                                                                   appSettings.CosmosDBKey);
                                                                                                     });

        public static IDocumentClient DocumentClient = LazyDocumentClient.Value;

        private static Lazy<IMyDisposable> LazyMyDisposable => new Lazy<IMyDisposable>(() => new MyDisposable());

        public static IMyDisposable MyDisposable => LazyMyDisposable.Value;
    }

    public interface IMyDisposable
    {
        void DoThings();
    }

    public class MyDisposable : IMyDisposable, IDisposable
    {
        private bool _disposed;

        public void DoThings()
        {
            CheckDisposed();

            // Do things
        }

        public void Dispose()
        {
            CheckDisposed();

            _disposed = true;
        }

        private void CheckDisposed()
        {
            if (_disposed)
            {
                throw new ObjectDisposedException("Bang");
            }
        }
    }

    public class SingletonDisposeTest
    {
        private readonly IMyDisposable _myDisposable;
        private readonly IDocumentClient _documentClient;

        public SingletonDisposeTest(IMyDisposable myDisposable, 
                                    IDocumentClient documentClient)
        {
            _myDisposable = myDisposable ?? throw new ArgumentNullException(nameof(myDisposable));
            _documentClient = documentClient ?? throw new ArgumentNullException(nameof(documentClient));
        }

        [FunctionName(nameof(SingletonDisposeTest))]
        public async Task<IActionResult> Run([HttpTrigger(AuthorizationLevel.Function, "get")]
                                             HttpRequest req,
                                             ILogger logger)
        {
            // This works fine
            _myDisposable.DoThings();

            // This does not
            var appSettings = new AppSettings();

            var uri = UriFactory.CreateDocumentCollectionUri(appSettings.CosmosDBName, 
                                                             appSettings.CosmosDBCollection);

            var documents = _documentClient.CreateDocumentQuery(uri)
                                           .ToArray();

            return new OkResult();
        }
    }

    public class Startup : FunctionsStartup
    {
        public override void Configure(IFunctionsHostBuilder builder)
        {
            builder.Services.AddLogging();
            builder.Services.AddSingleton(MySingletons.MyDisposable);
            builder.Services.AddSingleton(MySingletons.DocumentClient);
        }
    }

I don't quite understand why MyDisposable is ok and DocumentClient is not. Is there some special handling of DocumentClient or something?

soninaren commented 4 years ago

@brettsam any idea what is going on here?

@mark-r-cullen , Thanks for the additional information, i do want to understand why you are choosing to inject documentClient instead of using cosmosDbBinding. That way you would not have to manage the state of documentClient and you should still be able to access rich types.

mark-r-cullen commented 4 years ago

It is old code that we are revisiting and we are not really in a position to do this yet.

We just moved to a V2 function and switched from SimpleInjector to the built in DI, which is when we ran in to this issue. Would like to understand what's going on with it..

brettsam commented 4 years ago

I have no idea what's happening here -- I'm going to loop in @ealsur in case he knows of anything interesting about the DocumentClient specifically. The Functions host knows nothing about IDocumentClient so we don't do anything special with it.

Thanks for the concise sample, @mark-r-cullen, that's really helpful. @soninaren -- were you able to repro this with the sample code?

ealsur commented 4 years ago

DocumentClient implements IDisposable, there is nothing special. There is no code that would auto-dispose of a client instance. @mark-r-cullen Are you maybe using the DocumentClient inside a using clause in your code? Like:

using (DocumentClient client = <getting instance from DI>)
{
//
}

mark-r-cullen commented 4 years ago

@ealsur Nope, we're not disposing it anywhere ourselves (see the example I posted above).

I seem to remember we ran in to the same sort of issue with HttpClient, where the advise is to now use IHttpClientFactory. I don't really know the exact issue with HttpClient and the built in Function DI, could it be something similar or related to this perhaps?

mark-r-cullen commented 4 years ago

edit to tag @soninaren and @brettsam as I forgot, and to add a bit more info.

I have been investigating this some more and have figured out how to reproduce the issue. It is actually related to the function host not restarting properly when you change host.json / proxies.json it seems.

It actually has nothing to do with DocumentClient specifically. I just made a small mistake with my original sample code with the public static IMyDisposable MyDisposable / private static Lazy<IMyDisposable> LazyMyDisposable doing a => instead of =, which is why MyDisposable was working fine before. Sorry about that!

With the following code (v2 function app):

csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netcoreapp2.2</TargetFramework>
    <AzureFunctionsVersion>v2</AzureFunctionsVersion>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.Azure.Functions.Extensions" Version="1.0.0" />
    <PackageReference Include="Microsoft.Azure.WebJobs.Host.Storage" Version="3.0.13" />
    <PackageReference Include="Microsoft.Azure.WebJobs.Extensions.Storage" Version="3.0.11" />
    <PackageReference Include="Microsoft.NET.Sdk.Functions" Version="1.0.36" />
  </ItemGroup>
  <ItemGroup>
    <None Update="host.json">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
    <None Update="local.settings.json">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <CopyToPublishDirectory>Never</CopyToPublishDirectory>
    </None>
  </ItemGroup>
</Project>

cs

    public class Startup : FunctionsStartup
    {
        public override void Configure(IFunctionsHostBuilder builder)
        {
            builder.Services.AddLogging();

            builder.Services.AddSingleton(Singletons.MyDisposable);
        }
    }

    public static class Singletons
    {
        private static readonly Lazy<IMyDisposable> LazyMyDisposable = new Lazy<IMyDisposable>(() => new MyDisposable());

        public static IMyDisposable MyDisposable = LazyMyDisposable.Value;
    }

    public interface IMyDisposable : IDisposable
    {
        void DoThings();
    }

    public class MyDisposable : IMyDisposable
    {
        private bool _disposed;

        public void DoThings()
        {
            CheckDisposed();

            // Do things
        }

        public void Dispose()
        {
            if (!_disposed)
            {
                _disposed = true;
            }
        }

        private void CheckDisposed()
        {
            if (_disposed)
            {
                throw new ObjectDisposedException("Bang");
            }
        }
    }

    public class DisposeIssue
    {
        private readonly IMyDisposable _myDisposable;

        public DisposeIssue(IMyDisposable myDisposable)
        {
            _myDisposable = myDisposable;
        }

        [FunctionName(nameof(DisposeIssue))]
        public async Task<IActionResult> Run(
            [HttpTrigger(AuthorizationLevel.Anonymous, "get", "post", Route = null)]
            HttpRequest req,
            ILogger log)
        {
            _myDisposable.DoThings();

            return new OkResult();
        }
    }

Deploy it and hit the end point
You will get a 200 response
Make a change to either host.json or proxies.json
Hit the end point again after a few seconds
You will get a 500 response and app insights will show it's throwing the "Bang!" exception
Restart the function app from the overview blade
Hit the end point again after a few seconds
You will get a 200 response again

Please let me know if you can reproduce this now.

Azure / azure-functions-host