Reload token doesn't trigger on file deletion

[victorhurdugaci]

From scenario: https://github.com/aspnet/Release/issues/66

Run this app:

ConfigurationBuilder builder = new ConfigurationBuilder();
builder.AddJsonFile(s =>
{
    s.Path = "appsettings.json";
    s.ReloadOnChange = true;
});
var config = builder.Build();
var token = config.GetReloadToken();
token.RegisterChangeCallback(_ =>
{
    Console.WriteLine("Changed");
}, null);

while(true)
{
    Thread.Sleep(1000);
}

1. Create a copy of appsettings.json somewhere and make some changes to the copy.
2. Delete the original appsettings.json (notice that the token doesn't fire. expected?)
3. Copy the copied settings file back where the original appsettings.json was

Expected: token fires because the has settings have been updated
Actual: nothing happens

[HaoK]

Yeah I can repro this in a unit test, deleting files for some reason doesn't fire change notification, but writing to the file does.

[HaoK]

Looks like a bug in FileProviders with the watcher not firing on delete, as the following test fails, I will open an issue in FileSystems repo

        [Fact]
        public void DeletingFileWillFire()
        {
            var fileProvider = new PhysicalFileProvider(_basePath);

            var token = fileProvider.Watch("test.txt");

            Assert.False(token.HasChanged);

            File.WriteAllText(Path.Combine(_basePath, "test.txt"), @"{""JsonKey1"": ""JsonValue1""}");
            Assert.True(token.HasChanged);

            var token2 = fileProvider.Watch("test.txt");

            Assert.False(token2.HasChanged);

            File.Delete(Path.Combine(_basePath, "test.txt"));
            Assert.True(token2.HasChanged, "Deleted");
        }

[HaoK]

Tracked in FileSystem issue

[victorhurdugaci]

Reopening this because the bug is in Configuration not FileSystem. The problem is here:

Configuration/src/Microsoft.Extensions.Configuration.FileExtensions/FileConfigurationProvider.cs

Line 50 in 30e59f1

throw new FileNotFoundException($"The configuration file '{Source.Path}' was not found and is not optional.");

The file system watches triggers on file deletion and the code highlighted above is executed (reloading the configuration). The problem is that the file doesn't exist anymore and that code fails when the configuration file is not there and it's not optional.

The exception is then silently swollen by this code: https://github.com/aspnet/FileSystem/blob/98ea34e732734413da699959dd0342033de8c149/src/Microsoft.Extensions.FileProviders.Physical/PhysicalFilesWatcher.cs#L225 and the configuration provider acts like nothing has changed.

This scenario probably needs a redesign. Should the configuration provider not fail if the configuration file was loaded once before (aka on reload) (aka becoming an optional source)? Should you be able to watch for files that don't exist until they show up?

[HaoK]

I don't think its unreasonable to require optional: true for the scenario where deleting files is supported, it looks like things work now for deleting files if optional true. This test now passes:

        [Fact]
        public void DeletingFileWillReload()
        {
            // Arrange
            File.WriteAllText(Path.Combine(_basePath, "reload.json"), @"{""JsonKey1"": ""JsonValue1""}");
            File.WriteAllText(Path.Combine(_basePath, "reload.ini"), @"IniKey1 = IniValue1");
            File.WriteAllText(Path.Combine(_basePath, "reload.xml"), @"<settings XmlKey1=""XmlValue1""/>");

            var config = new ConfigurationBuilder()
                .AddIniFile("reload.ini", optional: true, reloadOnChange: true)
                .AddJsonFile("reload.json", optional: true, reloadOnChange: true)
                .AddXmlFile("reload.xml", optional: true, reloadOnChange: true)
                .Build();

            Assert.Equal("JsonValue1", config["JsonKey1"]);
            Assert.Equal("IniValue1", config["IniKey1"]);
            Assert.Equal("XmlValue1", config["XmlKey1"]);

            var token = config.GetReloadToken();

            // Act & Assert
            // Delete files
            File.Delete(Path.Combine(_basePath, "reload.json"));
            File.Delete(Path.Combine(_basePath, "reload.ini"));
            File.Delete(Path.Combine(_basePath, "reload.xml"));

            Thread.Sleep(1000);

            Assert.Null(config["JsonKey1"]);
            Assert.Null(config["IniKey1"]);
            Assert.Null(config["XmlKey1"]);
            Assert.True(token.HasChanged);
        }

@divega thoughts? That said, deleting files when optional: false basically means you would have to write an empty file instead of delete to clear one source when optional is false.

[victorhurdugaci]

I think the code should throw if the config file is deleted when optional = false. Otherwise, you get this silent bug and you have no idea that the configuration didn't change

[HaoK]

Well the configuration code is already throwing but the filewatcher is swallowing the exception right? Although I'm not sure you really want uncaught exceptions coming from the filewatcher either

[divega]

Let me list the options I think we have:

1. Check for file existence if optional: false is passed only the first time we load the file. If the file is deleted while we are watching we treat it as an empty file.
2. With optional: false + reloadOnChanged: true deleting the file becomes a kill switch, i.e. the only alternative for the exception being ignored as far as l understand it is to force the application to crash (similarly to how it would have crashed if the file wasn't there from the start)
3. Optional: false + reloadOnChanged: true becomes a non-valid/unsupported combination, and we throw at runtime if we detect it.

Anything I am not getting right or any option I am missing?

If these are the options I would be inclined to pick option 1, as my understanding is that optional: false is intended to be just a simple safeward to avoid forgetting required configuration files on deployment and turning it into a kill switch seems to take it too far.

[HaoK]

Okay if we change required files to only need to exist initially, that is a pretty simple fix. Sounds good to me

[divega]

I guess this is what @victorhurdugaci suggested at #433 (comment). He also made another very good point:

Should you be able to watch for files that don't exist until they show up?

It seems to me that after startup, files can appear, disappear and reappear. Are we setup to handle all cases?

[HaoK]

We currently are at the mercy of whatever the implementation of IFileProvider.Watch does in these cases, we basically just wire up that call to get the IChangeToken, and link it to Load().

I don't think we can do much more than explicitly block some of those scenarios (but I don't think we should)

[divega]

I think it is worth understanding how it will work. E.g. if an optional file is originally missing, do we still setup the file watcher in case it shows up later?

[HaoK]

We setup the file watching in the ctor if ReloadOnChange is true and we have a non null FileProvider, so if the provider supports notifications on a new file, that should work today. I can add a test to verify.

[HaoK]

@victorhurdugaci So even though configuration reload events are firing on deletion, the unit test I wrote against the FileProvider watch apis still fails on Delete, is there something specific in the usage there that I'm doing wrong?

        [Fact]
        public void DeletingFileWillFire()
        {
            var fileProvider = new PhysicalFileProvider(_basePath);

            var token = fileProvider.Watch("test.txt");
            Assert.False(token.HasChanged);
            File.WriteAllText(Path.Combine(_basePath, "test.txt"), @"{""JsonKey1"": ""JsonValue1""}");
            Assert.True(token.HasChanged);

            var token2 = fileProvider.Watch("test.txt");
            Assert.False(token2.HasChanged);
            File.Delete(Path.Combine(_basePath, "test.txt"));
            Thread.Sleep(1000);
            var called = false;
            token2.RegisterChangeCallback(_ => called = true, state: null);
            Assert.True(called); // fails
            Assert.True(token2.HasChanged, "Deleted"); // fails as well
        }

[HaoK]

eae76e3