Skip to content

Npgsql: don't create more than one DataSource#2045

Merged
unaizorrilla merged 10 commits intoXabaril:masterfrom
adamsitnik:npgSqlPerf
Oct 3, 2023
Merged

Npgsql: don't create more than one DataSource#2045
unaizorrilla merged 10 commits intoXabaril:masterfrom
adamsitnik:npgSqlPerf

Conversation

@adamsitnik
Copy link
Copy Markdown
Collaborator

@adamsitnik adamsitnik commented Sep 21, 2023

3 out of 4 DI extension methods are now ensuring that the DataSource is created only once (in a thread-safe way), and when it's possible they reuse the instance registered in the DI.

It's not an ideal fix, because the current design still allows the users to have more than one NpgsqlDataSource per their app. Example:

void Configure(IHostApplicationBuilder builder)
{
    builder.Services.AddNpgsqlDataSource("connectionString"); // registers a singleton factory for NpgsqlDataSource using Npgsql.DependencyInjection package, is used by the app
    builder.AddHealthChecks().AddNpgSql(new NpgSqlHealthCheckOptions()
    {
        ConnectionString = "connectionString" // registers a factory used only by the health check
    });
}

@unaizorrilla it's the fix I propose for 7.0.

For 8.0 we need to introduce a breaking change similar to what I did for the Azure clients in #2040

Contributes to #1993.

@adamsitnik adamsitnik added this to the V 7.1 milestone Sep 21, 2023
@github-actions github-actions bot added the test label Sep 21, 2023
@unaizorrilla unaizorrilla merged commit 418903e into Xabaril:master Oct 3, 2023
@adamsitnik adamsitnik deleted the npgSqlPerf branch October 16, 2023 07:44
@sungam3r
Copy link
Copy Markdown
Collaborator

sungam3r commented Dec 3, 2023

Current fix looks like more a hack than general-purpose solution.

@adamsitnik
Copy link
Copy Markdown
Collaborator Author

Current fix looks like more a hack than general-purpose solution.

I agree, that is why I wrote:

For 8.0 we need to introduce a breaking change similar to what I did for the Azure clients in

Related: #2096

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 89.79592% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.78%. Comparing base (a39d9fc) to head (796f47f).
⚠️ Report is 119 commits behind head on master.

Files with missing lines Patch % Lines
...ncyInjection/NpgSqlHealthCheckBuilderExtensions.cs 89.79% 4 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2045      +/-   ##
==========================================
- Coverage   66.72%   65.78%   -0.94%     
==========================================
  Files         262      174      -88     
  Lines        8439     4142    -4297     
  Branches      594      404     -190     
==========================================
- Hits         5631     2725    -2906     
+ Misses       2656     1325    -1331     
+ Partials      152       92      -60     
Flag Coverage Δ
AzureBlobStorage ?
AzureEventHubs ?
AzureFileStorage ?
AzureQueueStorage ?
AzureTableStorage ?
Dapr ?
Npgsql 3.53% <89.79%> (-25.20%) ⬇️
UI ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants