efcore.pg icon indicating copy to clipboard operation
efcore.pg copied to clipboard

Using NpgsqlDataSource in DependencyInjection pollutes other containers

Open eerhardt opened this issue 2 years ago • 2 comments

Running the following program:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Npgsql.EntityFrameworkCore.PostgreSQL" Version="8.0.0-rc.1" />
    <PackageReference Include="Npgsql.DependencyInjection" Version="8.0.0-preview.4" />
  </ItemGroup>

</Project>
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;

RunTest("Host=localhost;Database=test");
Console.WriteLine("First test passed");

RunTest("Host=myserver;Database=mydata");
Console.WriteLine("Second test passed");

static void RunTest(string connectionString)
{
    var serviceCollection = new ServiceCollection();

    serviceCollection.AddNpgsqlDataSource(connectionString);
    serviceCollection.AddDbContext<TestDbContext>(builder => builder.UseNpgsql(connectionString));

    var sp = serviceCollection.BuildServiceProvider();
    var context = sp.GetRequiredService<TestDbContext>();

    if (context.Database.GetDbConnection().ConnectionString != connectionString)
    {
        throw new Exception($"""
            This is broken.
            Expected: {connectionString}
            Actual:   {context.Database.GetDbConnection().ConnectionString}
            """);
    }
}

public class TestDbContext(DbContextOptions<TestDbContext> options) : DbContext(options) { }

produces an error on the 2nd test run:

First test passed
Unhandled exception. System.Exception: This is broken.
Expected: Host=myserver;Database=mydata
Actual:   Host=localhost;Database=test
   at Program.<<Main>$>g__RunTest|0_0(String connectionString) in C:\Users\eerhardt\source\repos\ConsoleApp100\ConsoleApp100\Program.cs:line 22
   at Program.<Main>$(String[] args) in C:\Users\eerhardt\source\repos\ConsoleApp100\ConsoleApp100\Program.cs:line 7

The connection string is reused across DI containers. As far as I can tell, it is because:

  1. EF uses the DbContextOptions as a cache key in here: https://github.com/dotnet/efcore/blob/5299be3bfeab62224f29aae9d4adff510878fcf7/src/EFCore/Internal/ServiceProviderCache.cs#L68-L71
  2. DbContextOptions overrides Equals: https://github.com/dotnet/efcore/blob/5299be3bfeab62224f29aae9d4adff510878fcf7/src/EFCore/DbContextOptions.cs#L142-L147 So DbContextOptions between 2 different DbContexts equal, and the stuff cached for the first context (like the INpgsqlSingletonOptions) are being reused across different DI containers

cc @roji

eerhardt avatar Oct 04 '23 16:10 eerhardt

Thanks @eerhardt, I'll look into this very soon.

roji avatar Oct 04 '23 18:10 roji

@eerhardt took me a while to get around to this 😅

In any case, the real fix for the root cause is #3063; am also thinking about other fix possibilities for 8.0.

roji avatar Jan 16 '24 10:01 roji

@eerhardt did a comprehensive overhaul of how DbDataSource is handled in the provider, all leaks should now be gone.

roji avatar May 18 '24 17:05 roji

Thanks for fixing this. I can confirm that #3043 is working properly now when using EF Core 9 preview 3 with the MyGet feed from https://www.myget.org/F/npgsql-vnext/api/v3/index.json (Npgsql.DependencyInjection v9.0.0-preview.1-ci.20240519T065219 and Npgsql.EntityFrameworkCore.PostgreSQL v9.0.0-preview.4-ci.20240518T175510).

Just wondering: is there any chance this will be backported to EF Core 8?

bkoelman avatar May 19 '24 08:05 bkoelman

@bkoelman thanks for testing and confirming!

As for backporting, #3167 is a pretty big PR with many changes, including some breaking changes around enum management; I unfortunately don't think it makes sense to backport.

roji avatar May 19 '24 08:05 roji

Ok sure, no problem. We'll wait until EF Core 9 then, before using this.

bkoelman avatar May 19 '24 10:05 bkoelman