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.

Sign up for GitHub

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

Jump to bottom

Reduce Environment.GetEnvironmentVariables calls on start-up #49856

Closed
wants to merge 3 commits into from
Closed

Reduce Environment.GetEnvironmentVariables calls on start-up #49856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa39359
Reduce Environment.GetEnvironmentVariables calls on startup
benaadams Mar 19, 2021
3172aa5
Feedback
benaadams Mar 19, 2021
59e2c0b
Merge branch 'main' into GetEnvironmentVariables
benaadams Mar 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion ...sions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ public class EnvironmentVariablesConfigurationProvider : ConfigurationProvider
private const string SqlServerPrefix = "SQLCONNSTR_";
private const string CustomPrefix = "CUSTOMCONNSTR_";

private static volatile IDictionary? s_environmentVariablesCache;

private readonly string _prefix;

/// <summary>
Expand All @@ -36,7 +38,7 @@ public EnvironmentVariablesConfigurationProvider(string prefix) =>
/// Loads the environment variables.
/// </summary>
public override void Load() =>
Load(Environment.GetEnvironmentVariables());
Load(s_environmentVariablesCache ?? CreateEnvironmentVariablesCache());

internal void Load(IDictionary envVariables)
{
Expand Down Expand Up @@ -112,5 +114,22 @@ private void AddIfPrefixed(Dictionary<string, string> data, string key, string v
}

private static string NormalizeKey(string key) => key.Replace("__", ConfigurationPath.KeyDelimiter);

private static IDictionary CreateEnvironmentVariablesCache()
{
IDictionary environmentVariables;
s_environmentVariablesCache = environmentVariables = Environment.GetEnvironmentVariables();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will misbehave if someone adds/sets an environment variable before Gen2 runs (which can be anytime). I think a better fix would be to cache it inside Environment class for Windows too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better fix would be to cache it inside Environment class for Windows too.

If it was in Environment would it need to do a defensive copy on return since its returning an IDictionary that is writable; but unlinked from environment vars? That would mean if it was called only once it would create 2 IDictionarys which would be a regression; could alternatively make it a live IDictionary so changes are reflected in Environment, but that might be breaking?

This will misbehave if someone adds/sets an environment variable before Gen2 runs

Might be bigger issues there?

  • The ConfigurationProvider has a Set method; however EnvironmentVariablesConfigurationProvider doesn't call Environment.SetEnvironmentVariable if that set method is called but just updates that instance's copy.

  • The ConfigurationProvider also has an IChangeToken which says to update and reload the configuration however if Environment.SetEnvironmentVariable is called it doesn't reload and fire that event (unlike with file config), nor is there currently an Environment event that it can listen to trigger that reload of configuration.

// We wait till Gen2 to cleanup as there might be a lot of allocations at startup.
Gen2GcCallback.Register((o) => CleanupEnvironmentVariablesCache(), environmentVariables);
return environmentVariables;
}

private static bool CleanupEnvironmentVariablesCache()
{
// Clear the cache
s_environmentVariablesCache = null;
// Stop running the Gen2 callback
return false;
}
}
}
4 changes: 4 additions & 0 deletions ...n.EnvironmentVariables/src/Microsoft.Extensions.Configuration.EnvironmentVariables.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
<EnableDefaultItems>true</EnableDefaultItems>
</PropertyGroup>

<ItemGroup>
<Compile Include="$(CoreLibSharedDir)System\Gen2GcCallback.cs" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Configuration\src\Microsoft.Extensions.Configuration.csproj" />
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Configuration.Abstractions\src\Microsoft.Extensions.Configuration.Abstractions.csproj" />
Expand Down