Avoid caching ProviderInfo.FullName at init time #11761
Conversation
|
@iSazonov Could you please update your PR description to include the summary and context? |
iSazonov
added
the
CL-General
|
@daxian-dbw Done. |
|
Looking at the issue described, we might want to consider this for GA |
| moduleName, | ||
| name); | ||
| } | ||
| internal string GetFullName() => EvaluateFullName(Name, PSSnapInName, ModuleName); |
There was a problem hiding this comment.
It doesn't seem necessary to have the GetFullName function. You can call EvaluateFullName(Name, PSSnapInName, ModuleName) directly instead, no?
There was a problem hiding this comment.
It is called in SessionStatePrividerAPIs too.
There was a problem hiding this comment.
If that's the case, can you maybe do this?
internal string GetFullName()
{
string result = Name;
if (!string.IsNullOrEmpty(PSSnapInName))
{
result =
string.Format(
System.Globalization.CultureInfo.InvariantCulture,
"{0}\\{1}",
PSSnapInName,
Name);
}
// After converting core snapins to load as modules, the providers will have Module property populated
else if (!string.IsNullOrEmpty(ModuleName))
{
result =
string.Format(
System.Globalization.CultureInfo.InvariantCulture,
"{0}\\{1}",
ModuleName,
Name);
}
return result;
}There was a problem hiding this comment.
Yes, I can. Will do.
| psSnapInName, | ||
| name); | ||
| } | ||
| return _fullName ?? (_fullName = GetFullName()); |
There was a problem hiding this comment.
The name is still cached. But does it make sens to caching this name at all?
There was a problem hiding this comment.
This comes from #8831 where we add the optimization. It is amazing to calculate the full name for every provider operation. I'd do not revert the optimization.
There was a problem hiding this comment.
So it's not a problem to use the cached name in other places but only those 2 spots you changed in this PR?
There was a problem hiding this comment.
It is important to do not cache the provider full name in the submodule until we set root module name.
There was a problem hiding this comment.
Then it would be very helpful to add comments to the place where you use GetFullName() instead of the cached fullname, otherwise, another refactoring in future might change it back to use the cached value.
There was a problem hiding this comment.
The comment is already there "Use GetFullName() to avoid caching full name at init time."
There was a problem hiding this comment.
Ah, yes, I missed that. Can you please also add the information about why to avoid the cached name?
|
@iSazonov Please add tests for the change. There is no urgency to take this in 7.0 GA since it is not a regression from 6.2. We will consider this for GA if there are tests added to this PR and its merged by 10 am PDT on 02/12/2020. Otherwise we will consider this for the next servicing release of 7. /cc @SteveL-MSFT |
|
@adityapatwardhan @daxian-dbw @rjmholt Current fix is not reliable - later someone might accidentally violate these implicit conditions for using the cached name. I make more reliable fix in #11813 and add a test. |
ghost
added
the
Waiting on Author
|
@iSazonov Can this PR be closed? |
|
removing from GA-Consider to remove to from triage until it is ready to review |
|
I think #11813 is better fix. |
ghost
removed
the
Waiting on Author
5 participants
PR Summary
Address #9840
In #8831 we added caching for ProviderInfo.FullName. It came a problem in scenario of loading nested module. PowerShell change nested module name to root module name after it is loaded the nested module. In the scenario ProviderInfo.FullName is cached with nested module name and not updated to root module name.
The fix is to do not cache ProviderInfo.FullName at init time. After modules are loaded and cached name is safely used.
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.