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

Add failing test that exposes DbDataSource related bug. #3003

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lauxjpn
Copy link

@lauxjpn lauxjpn commented Dec 3, 2023

@roji I took your DbDataSource support implementation as a base for our Pomelo implementation and came across a subtle bug.

This PR adds a test that fails.

The issue is that an old DbDataSource instance might be used by a later DbContext instance, if both contexts (the previous and the later one) are supposed to use a DbDataSource instance that was added as a service to the application service provider.

The reason seems to be that NpgsqlOptionsExtension is cached by the service provider cache and later reused, because none of its properties have changed (DataSource is null in both cases (the previous and the later context), since the DbDataSource instances are added via DI).

Because NpgsqlOptionsExtension is cached, no new NpgsqlSingletonOptions instance is created. But because it is NpgsqlSingletonOptions.Initialize() where the application service provider is checked for a DI supplied DbDataSource instance, later calls then reuse the obsolete one from the time that NpgsqlOptionsExtension was cached.

We workaround this issue by checking for DbDataSource services at the time we would actually need to, which is in our IRelationalConnection implementation. This might not be a good enough workaround for you, because you are accessing your INpgsqlSingletonOptions.DataSource property also from NpgsqlTypeMappingSource.

(Using INpgsqlSingletonOptions.ApplicationServiceProvider would suffer from the same issue.)

(It might be possible to pass the CoreOptionsExtension instance or the DbContextOptions instance as an option (With...) to NpgsqlOptionsExtension and then check the service provider(s) it contains in the GetServiceProviderHashCode()/ShouldUseSameServiceProvider() methods so that the NpgsqlOptionsExtension instance doesn't get cached if the NpgsqlDataSource service (and/or the service provider?) changed. I did not analyze or test this in any way, however, since it seems that Pomelo works fine with our current implementation. So this might not work or might turn out to be unreliable (e.g. somebody might alter the immutable CoreOptionsExtension instance after it has been passed, resulting in a new/cloned final instance). Anyway, just throwing it out there.).

@roji
Copy link
Member

roji commented Dec 4, 2023

Thanks @lauxjpn, I'll take a look at this (probably will need a bit of time).

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

Successfully merging this pull request may close these issues.

2 participants