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

ServiceProviderCache and DataSource usage - leads to creation of many Service Providers in integration tests #2720

Closed
plachor opened this issue Apr 14, 2023 · 7 comments

Comments

@plachor
Copy link

plachor commented Apr 14, 2023

Hi, I am upgrading my project to Npgsql7 along with EF7. In previous version I registered Npgsql with connection string.

options.UseNpgsql("SOME_CONNECTION_STRING", b =>{});

Following your release notes I've tried to move to overload accepting DataSource. So using NpgsqlDataSourceBuilder I create instance of DataSource and try to reuse it for application lifetime. So I call:

options.UseNpgsql(dataSourceInstance, b =>{});

In integration tests I have many collection fixtures that are covering different scenarios in parallel with some separation of data. This was the time I released you construct internal DI for EF and you have some static cache that is limiting those DIs to the count of 20.

It seems that when application is relaying on connection string your implementation of https://github.com/npgsql/efcore.pg/blob/main/src/EFCore.PG/Infrastructure/Internal/NpgsqlOptionsExtension.cs#L418 is ignoring changes in connection string. So each time same service provider is being served for different connection string (different db).

On the other hand if you relay DataSource this counts in and since each collection has own connection string (custom-db) so they will receive it's own ServiceProvider from ServiceProviderCache.

  1. It seems that similar should happen for connectionString based approach, is that bug or intentional ?
  2. What should I do to remove this limitation of 20 providers in integration tests where I construct more than 20 tests servers with dedicated data-bases that were created on demand when fixture is constrcuted.
@roji
Copy link
Member

roji commented Apr 14, 2023

tl;dr this is expected, and it should be fine to ignore the EF warning on service providers - in your tests.

EF allows you to change connection strings without creating a new service provider, since as far as EF is concerned the connection string doesn't actually change anything; the different connection pools for different conneciton strings are managed at a lower level inside the ADO.NET driver, but EF doesn't care about it. NpgsqlDataSource, on the other hand, represents more than just a connection string, and is represented as a singleton thing inside EF.

/cc @ajcvickers

@plachor
Copy link
Author

plachor commented Apr 17, 2023

Thnx @roji for your response, but re-configuring my integration tests to ignore this will lead me to situation in which I will find out I messed something in registration on production environment if unlucky.

Any static state is Achilles heel in case of integration tests which share those statics.

@ajcvickers I know this is not EF repository but question for you: could this internal ServiceProvider be stored witin DataSource in case you detect it was being used to initialize connection ?

It seems that this way you will achieve same and not need any static cache implicitly shared by separated domains. I guess it can happen not only in case of integration tests. I can imagine someone can just have many processes being orchestrated by .NET code base each with its own DI and liberty to use any database.

@roji
Copy link
Member

roji commented Apr 17, 2023

Thnx @roji for your response, but re-configuring my integration tests to ignore this will lead me to situation in which I will find out I messed something in registration on production environment if unlucky.

How so? It's common for the testing configuration to vary in certain minor ways - after all the connection string you're using in testing isn't the same as the one you're using in production. Simply disabling this warning seems very reasonable and low-risk. If you're specifically concerned about a bug in your code which would cause too many service providers to be created in production, you can write a specific test for that where the warning wouldn't be disabled.

@ajcvickers I know this is not EF repository but question for you: could this internal ServiceProvider be stored witin DataSource in case you detect it was being used to initialize connection ?

DataSource works at a lower level (ADO.NET, System.Data) and has nothing to do with EF. So that's not possible. Also, different service providers may need to be created even if you're using the same DataSource; for example, some EF configuration detail may require a different service provider.

@plachor
Copy link
Author

plachor commented Apr 18, 2023

I believe static instances should be avoided when only possible. But will do so in my tests.

One more question, regarding usage of:

options.UseNpgsql(dataSourceInstance, b =>{
    b.UseNodaTime();
});

is there good way of configuring UseNodaTime along with such preinitialized dataSource ? I tried to invoke UseNodaTime on DataSourceBuilder however that is not working for me. I end with :

The 'Instant' property `PROPERTY_NAME' could not be mapped because the database provider does not support this type.

When calling dbContext.Database.MigrateAsync();

@roji
Copy link
Member

roji commented Apr 18, 2023

Right now, when doing a data source with the EF provider, you must configure NodaTime at both the ADO.NET layer (on NpgsqlDataSourceBuilder) and at the EF layer (in UseNpgsql, as you're doing above). I've got some plans for improving this in #2542, but that's how it is at the moment.

The error above indicates that NodaTime likely wasn't configured at the EF layer - I can't really help you on that without seeing some code. Try putting together a minimal, runnable repro for this and you'll likely run into the issue while doing so.

@plachor
Copy link
Author

plachor commented Apr 19, 2023

Thnx @roji, configuring it at both levels is working for me, I assumed I should move to dataSourceBuilder registration only.

@roji
Copy link
Member

roji commented Apr 19, 2023

Happy to hear it worked out, and yeah - the current experience isn't perfect with the double registration... I'll hopefully be able to make it better at some point.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Apr 19, 2023
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

2 participants