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

Each new NpgsqlDataSourceBuilder() call holds 1 more Database Connection forever and locks the DB eventually #3377

Closed
lansman opened this issue Nov 19, 2024 · 9 comments

Comments

@lansman
Copy link

lansman commented Nov 19, 2024

We have two Postgres databases: RW and RO. Accordingly, our AspNet core project uses two Entity Framework's DbContext: RW and RO. Each has its own connection string to the corresponding DB. Connection strings should not be hardcoded, they should be fetched from some function with some logic inside. This function should be called on each DbContext creation.

DbRwConnectionOptionsReader.cs

public class DbRwConnectionOptionsReader
{
    public string ReadConnectionString() { /* some logic including _configurationRoot.GetValue<string>("ConnectionString") etc*/ }
}

DbRoConnectionOptionsReader.cs

public class DbRoConnectionOptionsReader
{
    public string ReadConnectionString() { /* some logic */ }
}

There is a Registrator class contains all DI registrations, including DB registration

Registrator.cs

public class Registrator()
{
    private NpgsqlDataSource _dataSourceRo;
    private NpgsqlDataSource _dataSourceRw;
    private IServiceCollection _serviceCollection;
   
    public Registrator(IServiceCollection serviceCollection)
    {
        _serviceCollection = serviceCollection;
    }
    
    public void ConfigureDb()
    {
        _serviceCollection.AddDbContext<DbContext>(SettingsRw);
        _serviceCollection.AddDbContext<DbContextRo>(SettingsRo);
        _serviceCollection.AddDbContextFactory<DbContext>(SettingsRw);
        _serviceCollection.AddDbContextFactory<DbContextRo>(SettingsRo);
    }
    
    private void SettingsRw(IServiceProvider provider, DbContextOptionsBuilder builder)
    {
        builder.UseNpgsql(GetDataSource(ref _dataSourceRw, provider.GetRequiredService<DbConnectionOptionsReader>()));
    }
    
    private void SettingsRo(IServiceProvider provider, DbContextOptionsBuilder builder)
    {
        builder.UseNpgsql(GetDataSource(ref _dataSourceRo, provider.GetRequiredService<DbRoConnectionOptionsReader>()));
    }
    
    private NpgsqlDataSource GetDataSource(ref NpgsqlDataSource dataSource, IDbConnectionStringReader dbOptionsReader)
    {
        var newConnectionString = dbOptionsReader.ReadConnectionString();

        if (dataSource == null || dataSource.ConnectionString != newConnectionString) // btw these connection string are always unequal, cause first returns Server=, but seconds has Host=, gonna fix it later, but it doesn't matter for the current case
        {
            var dataSourceBuilder = new NpgsqlDataSourceBuilder(newConnectionString); // the problem is here
            dataSourceBuilder.UseJsonNet();
            //dataSourceBuilder.EnableDynamicJson();
            dataSourceBuilder.MapEnum<Enum1>("public.enum1");
            dataSourceBuilder.MapEnum<Enum2>("public.enum2");
            dataSource = dataSourceBuilder.Build();
        }

        return dataSource;
    }
}

It is created in Program.cs

var builder = WebApplication.CreateBuilder(args);
var registrator = new Registrator(builder.Services);
registrator.ConfigureDb();

The problem is each call

new NpgsqlDataSourceBuilder()

creates +1 Database connection hanging forever.
Eventually a Postgres Connection Limit is reached and API stops to serve any further requests with error
too many connections for role "role_name"
image

Jetbrains DotMemory analysis shows there are only 2 NpgsqlDataSourceBinder instances after many requests, which is totally fine (first is for the RW and second is for RO, I guess)
image

But there are 14 NpgsqlDataSourceConfiguration, they are not cleaned up by GC, why?
image

More dotMemory screenshots

image

image

image

image

image

@roji
Copy link
Member

roji commented Nov 20, 2024

The report above is confusing:

  1. Above you say that each new NpgsqlDataSourceBuilder "creates +1 Database connection hanging forever" (this shouldn't be possible, since NpgsqlDataSourceBuilder never creates database connections at all - it's only used to build an NpgsqlDataSource)
  2. Below you seem to complain about 14 NpgsqlDataSourceConfiguration instances.

So which is it, connections or NpgsqlDataSourceConfiguration?

But there are 14 NpgsqlDataSourceConfiguration, they are not cleaned up by GC, why?

When exactly something gets cleaned up by the GC depends on many factors, but objects do not get cleaned up right away; it's possible that a lot of time must pass, or that memory becomes low before the GC will kick in. Seeing 14 NpgsqlDataSourceConfiguration in the memory profiler is not, in itself, any indication of a memory leak or similar.

@lansman
Copy link
Author

lansman commented Nov 24, 2024

  1. Okay, it's something else creates and hold the connection.
  2. The main issue is holding connections. I'm trying to investigate that and figured out, there are a lot of NpgsqlDataSourceConfiguration, this doesn't look good, does it? Probably it's not the NpgsqlDataSourceConfiguration fault, something else holds both NpgsqlDataSourceConfiguration and DB connections. I can look for another objects if you suggest me some.

Regarding GC: DotMemory calls GC before creating each Snapshot (screenshots above show snapshot data). More than that, I also called GC manually by pressing the corresponding DotMemory button. Neigher NpgsqlDataSourceConfiguration nor DB connections were released after that.

There is no such problem when I rewrite the code and call new NpgsqlDataSourceBuilder only once, connections are being released and reused as they should be. So there is something wrong with continuous new NpgsqlDataSourceBuilder calls

@roji
Copy link
Member

roji commented Nov 24, 2024

So just to be clear: calling NpgsqlDataSourceBuilder.Build() builds an NpgsqlDataSource; this NpgsqlDataSource references a NpgsqlDataSourceConfiguration and stays "alive", and both will continue to be in memory until there are no references to the NpgsqlDataSource. Also, as NpgsqlDataSource is disposable, it should be disposed (via Dispose() or DispoesAsync()).

In general, applications are expected to only need to build a single NpgsqlDataSource for the entirety of the application (or possibly one-per-tenant, if they're multitenant and need different connection pools for each tenant). You seem to want to have two data sources - one for read-only usage and one for read-write - and that's fine; but in that case you should only ever be building two NpgsqlDataSources in your application.

So I'd concentrate on understanding exactly when you're building NpgsqlDataSources and why; looking at NpgsqlDataSourceConfiguration via DotMemory really is secondary and not the important thing here.

@lansman
Copy link
Author

lansman commented Nov 24, 2024

until there are no references to the NpgsqlDataSource

It doesn't work, I guess. Each subsequent call builder.UseNpgsql(dataSource) accepting a new dataSource, should release the old one, shouldn't it? It looks like it continues to hold all previous dataSources forever.

So I'd concentrate on understanding exactly when you're building NpgsqlDataSources and why

I need to change ConnectionString time to time during API lifetime. What is the best way to implement that without memory objects/connection stuck issues?

@roji
Copy link
Member

roji commented Nov 24, 2024

Each subsequent call builder.UseNpgsql(dataSource) accepting a new dataSource, should release the old one, shouldn't it?

No. As I wrote above, you should not be calling UseNpgsql() with a new, different dataSource every time - doing so will cause severe performance problems (each data source represents a connection pool, so this effectively disables connection pooling).

What you need to do, is have one or maybe two global/singleton data source instances, period. You can register these data sources as singletons in your DI container (as keyed services), or use some other mechanism to manage them.

Note that if you're using 9.0, it's recommended to not configure and instantiate your NpgsqlDataSources outside of EF, and then pass them to UseNpgsql(); instead, use the new ConfigureDataSource() API (see the release notes).

@hugh-maaskant
Copy link

@roji Is it possible to obtain the NpgsqlDataSource from the configured DbContext? I'm asking as I have some queries that are using Dapr and it would be nice to have them use the same NpgsqlDataSource and hence (if I understand you correctly) the same connection pool. I looked at the documentation, but could not find it (but that does nor mean it is not there of course)

Thanks

@roji
Copy link
Member

roji commented Nov 25, 2024

DbContext doesn't itself expose the NpgsqlDataSource it was configured with, no. But if you also have external, non-EF usage (e.g. Dapper), then I'd advise registering your NpgsqlDataSources in DI (or both of them - via keyed services), and then using that same data source in both EF (passing it to UseNpgsql) and Dapper. That's the simple, recommended route, rather than trying to have EF manage your NpgsqlDataSource and then extracting it out for Dapper.

I'll go ahead and close this as everything seems to be working by design. But if you have further questions don't hesitate to post back here.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Nov 25, 2024
@hugh-maaskant
Copy link

Thanks @roji, I can do that easily, only passing the NpgsqlDataSource to EF Core was not recommended (in the docs), so I figured I'd better ask the expert ;-)

@roji
Copy link
Member

roji commented Nov 25, 2024

Sure. If you're only using EF, definitely use ConfigureDataSource(). But if you really do need to share a data source between EF and something else, it's perfectly fine to build your data source externally, and pass it to EF. You just have to make sure that your data source and EF configuration align (e.g. enums and plugins must be set up in the same way at both layers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants