-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Description
Is there an existing issue for this?
- I have searched the existing issues
Describe the bug
When configuration looks like:
appsettings.json
appsettings.Staging.json
appsettings.Production.json
It is possible to skip loading appsettings.json and only load appsettings.Production.json; leaving the server in an unexpected state.
Loading configuration files:
foreach (var configFile in configFiles)
{
if (File.Exists(configFile))
{
var appSettingsJson = File.ReadAllBytes(configFile);
// Perf: Using this over AddJsonStream. This allows the linker to trim out the "File"-specific APIs and assemblies
// for Configuration, of where there are several.
Configuration.Add<JsonStreamConfigurationSource>(s => s.Stream = new MemoryStream(appSettingsJson));
}
}
This code is wrong. Never call File.Exists in server code; it does the wrong thing. The problem is it returns false on IO errors; which is fine for its original use case of checking if the user passed a path to a removable drive that currently doesn't have a disk in it; but is really bad on network backed disks.
The problem is you can get a spurious false at any time from File.Exists because you have a transient network glitch causing access to the disk to return an IO error; but the next access succeeds. I have lost data due this case in the past. File.Exists assumes the disk is physically present in the machine it is called on, and therefore all IO errors are repeatable.
Expected Behavior
Logically speaking this wants to be
try { File.ReadAllBytes(configFile) } catch (FileNotFoundException) {}
but that causes problems for the developer; an exotic solution is required to avoid breaking in the IDE when the file really doesn't exist.
The following is not to my taste but would work:
if (applicationEnvironment == "Development")
{
if (File.Exists(configFile))
{
var appSettingsJson = File.ReadAllBytes(configFile);
// Perf: Using this over AddJsonStream. This allows the linker to trim out the "File"-specific APIs and assemblies
// for Configuration, of where there are several.
Configuration.Add<JsonStreamConfigurationSource>(s => s.Stream = new MemoryStream(appSettingsJson));
}
}
else
{
// Don't call File.Exists in production; it might be network-attached storage.
try
{
var appSettingsJson = File.ReadAllBytes(configFile);
// Perf: Using this over AddJsonStream. This allows the linker to trim out the "File"-specific APIs and assemblies
// for Configuration, of where there are several.
Configuration.Add<JsonStreamConfigurationSource>(s => s.Stream = new MemoryStream(appSettingsJson));
}
catch (FileNotFoundException)
{
// That's fine. The configuration that exists will be loaded.
}
}
A more concise alternative is to provide a server-alternative to File.Exists() and call that. It's only a handful of lines, but it belongs somewhere where everybody can call it so that the bug eventually gets fixed everywhere not just here.
Looks something like this:
if (File.ExistsServer(configFile))
{
var appSettingsJson = File.ReadAllBytes(configFile);
// Perf: Using this over AddJsonStream. This allows the linker to trim out the "File"-specific APIs and assemblies
// for Configuration, of where there are several.
Configuration.Add<JsonStreamConfigurationSource>(s => s.Stream = new MemoryStream(appSettingsJson));
}
In File.cs:
public static bool ExistsServer(string path)
{
var info = new FileInfo(path);
_ = info.LastWriteTimeUtc; // Value doesn't matter. Merely accessing the property suffices to throw IO errors.
return info.Exists;
}
Steps To Reproduce
A controlled reproduction requires fault injection.
Exceptions (if any)
None. Should throw IOException but silently doesn't load configuration file instead.
.NET Version
10.0.1
Anything else?
No response