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

Using NpgsqlDataSource in DependencyInjection pollutes other containers #2891

Closed
eerhardt opened this issue Oct 4, 2023 · 6 comments · Fixed by #3167
Closed

Using NpgsqlDataSource in DependencyInjection pollutes other containers #2891

eerhardt opened this issue Oct 4, 2023 · 6 comments · Fixed by #3167
Assignees
Labels
bug Something isn't working
Milestone

Comments

@eerhardt
Copy link

eerhardt commented Oct 4, 2023

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

@roji
Copy link
Member

roji commented Oct 4, 2023

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

eerhardt added a commit to dotnet/aspire that referenced this issue Oct 10, 2023
eerhardt added a commit to dotnet/aspire that referenced this issue Oct 13, 2023
* Delay connection validation

When an app is deployed to a new environment, it is common for the connection information to be missing initially. When this is the case, the app crashes very early (inline when calling AddXXX for the component). When throwing this early, there isn't an ILogger which can log the exception, and thus the exception is written to stderr/stdout by the .NET runtime. This isn't ideal since certain environments expect stdout logs to be in a certain format.

The fix is to delay the connection validation until the underlying DI service is requested/created. At this point the DI container is built, and the app can log an exception with the configured ILogger.

Fix #185

* Work around npgsql/efcore.pg#2891
eerhardt added a commit to dotnet/aspire that referenced this issue Nov 6, 2023
This test randomly fails because of npgsql/efcore.pg#2891. Disabling it until that bug is fixed.

Fix #496
Contributes to #365
davidfowl pushed a commit to dotnet/aspire that referenced this issue Nov 7, 2023
This test randomly fails because of npgsql/efcore.pg#2891. Disabling it until that bug is fixed.

Fix #496
Contributes to #365
@roji roji self-assigned this Jan 15, 2024
@roji roji added the bug Something isn't working label Jan 15, 2024
@roji roji added this to the 9.0.0 milestone Jan 16, 2024
@roji
Copy link
Member

roji commented Jan 16, 2024

@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 added a commit to roji/efcore.pg that referenced this issue May 12, 2024
roji added a commit to roji/efcore.pg that referenced this issue May 13, 2024
roji added a commit to roji/efcore.pg that referenced this issue May 13, 2024
roji added a commit to roji/efcore.pg that referenced this issue May 17, 2024
roji added a commit that referenced this issue May 18, 2024
@roji
Copy link
Member

roji commented May 18, 2024

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

@bkoelman
Copy link

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?

@roji
Copy link
Member

roji commented May 19, 2024

@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.

@bkoelman
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants