Closed adams85 closed 1 year ago
This implementation, in its current state, has a pretty obscure breaking change.
Consider the following situation: consumer has a multi-threaded application (like an ASP.NET Core web application) and use a naive way in their application to interact with ConfigCat by creating and disposing ConfigCatClient
instances per request. Such an implementation could result in an execution like this (strongly simplified):
/* 1. [Request1/Thread1] */ var client1 = new ConfigCatClient(sdkKey: "123");
/* 2. [Request2/Thread2] */ var client2 = new ConfigCatClient(sdkKey: "123");
/* 3. [Request1/Thread1] */ var flag1 = client1.GetValue(...);
/* 4. [Request1/Thread1] */ client1.Dispose();
/* 5. [Request2/Thread2] */ var flag2 = client2.GetValue(...);
/* 6. [Request2/Thread2] */ client2.Dispose();
Although this is highly suboptimal, it works correctly. However, by introducing the new API which returns a shared instance per SDK Key this code is not correct any more if consumer just updates their code "blindly":
/* 1. [Request1/Thread1] */ var client1 = ConfigCatClient.Get(sdkKey: "123");
/* 2. [Request2/Thread2] */ var client2 = ConfigCatClient.Get(sdkKey: "123");
/* 3. [Request1/Thread1] */ var flag1 = client1.GetValue(...);
/* 4. [Request1/Thread1] */ client1.Dispose();
/* 5. [Request2/Thread2] */ var flag2 = client2.GetValue(...);
/* 6. [Request2/Thread2] */ client2.Dispose();
In this case, client1
and client2
will hold a reference to the same ConfigCatClient
instance. Step 4 disposes of/close the shared instance and from step 5 on methods are called on a disposed instance, which is pretty much an undefined behavior that could result in subtle bugs.
After discussing this issue, we decided that this will be the intended behavior and the fact that ConfigCatClient.Get
returns a shared instance will be emphasized in the docs.
Kudos, SonarCloud Quality Gate passed!
Can you review the sonar's findings?
@endret I've updated the PR as per your remarks.
Can you review the sonar's findings?
- "Complete the task associated to this 'TODO' comment." issues: I mark comments with "TODO" which needs attention in the future, never push commits with unfinished code or something like that. There's a reason why I use "TODO" and not something else: Visual Studio has a nice feature called Task List which provides a convenient way to overview such comments and "by default, Visual Studio includes the following tokens: HACK, TODO, UNDONE, and UnresolvedMergeConflict".
GC.SuppressFinalize
related issues: calling this method inDisposeAll
is intentional. The root of the problem is thatDisposeAll
is not idiomatic in C#, so sonar is not prepared for that.- "Remove this unused method parameter 'disposing'." issue: this parameter is part of the standard dispose pattern.
- "Remove this hardcoded path-delimiter." issue: this code is not created by this PR, it's already present in the base branch.
Are there any further issues which needs to be addressed or we can go ahead with merging the PR?
Kudos, SonarCloud Quality Gate passed!
@endret I've updated the PR as per your remarks.
Can you review the sonar's findings?
- "Complete the task associated to this 'TODO' comment." issues: I mark comments with "TODO" which needs attention in the future, never push commits with unfinished code or something like that. There's a reason why I use "TODO" and not something else: Visual Studio has a nice feature called Task List which provides a convenient way to overview such comments and "by default, Visual Studio includes the following tokens: HACK, TODO, UNDONE, and UnresolvedMergeConflict".
GC.SuppressFinalize
related issues: calling this method inDisposeAll
is intentional. The root of the problem is thatDisposeAll
is not idiomatic in C#, so sonar is not prepared for that.- "Remove this unused method parameter 'disposing'." issue: this parameter is part of the standard dispose pattern.
- "Remove this hardcoded path-delimiter." issue: this code is not created by this PR, it's already present in the base branch.
Are there any further issues which needs to be addressed or we can go ahead with merging the PR?
Ok, thanks. Meanwhile, the weekRef added a complexity issue to the analysis.
Meanwhile, the weekRef added a complexity issue to the analysis.
Yeah, that complexity issue of the polling methods is a pain point... AutoPollConfigService.StartScheduler
is changed in almost all subsequent PRs so I suggest ignoring this for now and let's address it later (probably, in a separate PR) to avoid merge conflicts.
Thanks for the review!
Describe the purpose of your pull request
ConfigCatClient.Get
andConfigCatClient.DisposeAll
static methodsConfigCatClient.Dispose
to remove instance from cache.ConfigCatClient
ctorsConfigCatClientOptions.SdkKey
(ConfigurationBase.SdkKey
)Also,
Requirement checklist (only if applicable)