ChangeToken.OnChange fires twice when listening for configuration changes
Tratcher opened this issue · 9 comments
From @magnusbakken on March 20, 2017 10:22
When I use ChangeToken.OnChange to listen to configuration changes as described in aspnet/Configuration#432, my handler usually gets called twice in quick succession.
To reproduce:
- Create a new ASP.NET Core targeting .NET Framework project using the Web API template (I haven't tested targeting .NET Core).
- Add this line in the Configure method in Startup.cs:
ChangeToken.OnChange(Configuration.GetReloadToken, () => loggerFactory.CreateLogger<Startup>().LogWarning("Configuration changed")); - Start the app.
- While the app is running, make a change in appsettings.json and save the file.
The line "Configuration changed" shows up twice in the console output.
I've tried to make the file change from multiple programs (Visual Studio, Notepad, emacs), and by copying and replacing the file in its entirety. At least once I think I saw the change only being logged once, but usually you get it twice with just a few milliseconds between the log events.
I've attached the call stacks I see from the debugger for the first and second call respectively:
first-callstack.txt
second-callstack.txt
The only difference is the following five lines in the middle of the second stack:
mscorlib.dll!System.Threading.CancellationTokenSource.InternalRegister(System.Action<object> callback, object stateForCallback, System.Threading.SynchronizationContext targetSyncContext, System.Threading.ExecutionContext executionContext) Unknown
mscorlib.dll!System.Threading.CancellationToken.Register(System.Action<object> callback, object state, bool useSynchronizationContext, bool useExecutionContext) Unknown
mscorlib.dll!System.Threading.CancellationToken.Register(System.Action<object> callback, object state) Unknown
Microsoft.Extensions.Primitives.dll!Microsoft.Extensions.Primitives.CancellationChangeToken.RegisterChangeCallback(System.Action<object> callback, object state) Unknown
Microsoft.Extensions.Primitives.dll!Microsoft.Extensions.Primitives.ChangeToken.OnChange.AnonymousMethod__0(object s) Unknown
Copied from original issue: aspnet/Configuration#624
From @HaoK on June 15, 2017 23:45
Yeah this is just how change tokens work, what we did to somewhat mitgate this when we are listening for changes was to add a delay before invoking callback so the changes hopefully will be finished:
ChangeToken.OnChange(
() => Source.FileProvider.Watch(Source.Path),
() => {
Thread.Sleep(Source.ReloadDelay);
Load(reload: true);
});
From @HaoK on June 15, 2017 23:45
You likely need to do something similar if you don't want to get called multiple times for the same underlying change.
Note this is actually caused by the underlying file watcher triggering multiple times for the same change. The second call to Watch sometimes fires immediately because of this.
@divega @HaoK @Tratcher I'm coming up real quick on the topic for ChangeToken. I'll outline the topic to use ChangeToken with config files (as I'm mess'in around with below) and for general file watching. More on that later over in the doc issue. Right now, I'm just playing with it.
I couldn't get a reliable solution to the double-firing problem using a delay, so what do you cats think about a hash check approach? Even if fine for appsettings.json, this would require expansion to cover the appsettings.{environment}.json file, which I could roll in easily enough. [EDIT] Added some bits below to cover the environment version of the file.
public class Startup
{
private ConfigurationReloadToken _changeToken = new ConfigurationReloadToken();
private byte[] _configFileHash1 = new byte[20];
private byte[] _configFileHash2 = new byte[20];
public void ConfigureServices(IServiceCollection services)
{
...
}
public void Configure(IApplicationBuilder app, IHostingEnvironment env,
IConfiguration config)
{
ChangeToken.OnChange(
() => config.GetReloadToken(),
() =>
{
byte[] configFileHash1 = ComputeHash("appSettings.json");
byte[] configFileHash2 = ComputeHash($"appSettings.{env.EnvironmentName}.json");
if (!_configFileHash1.SequenceEqual(configFileHash1) ||
!_configFileHash2.SequenceEqual(configFileHash2))
{
DoSomething();
_configFileHash1 = configFileHash1;
_configFileHash2 = configFileHash2;
}
var previousToken =
Interlocked.Exchange(ref _changeToken, new ConfigurationReloadToken());
previousToken.OnReload();
});
...
}
private byte[] ComputeHash(string file)
{
if (File.Exists(file))
{
using (var fs = File.OpenRead(file))
{
return System.Security.Cryptography.SHA1.Create().ComputeHash(fs);
}
}
else
{
return new byte[20];
}
}
private void DoSomething()
{
Console.WriteLine($"********** CONFIG CHANGED! {DateTime.Now}");
}
}[EDIT] Also works fine for other files (as long as I don't use VS Code to save/update the file ... VSC seems to lock the file a little bit too long. Notepad seems better about not locking the file long).
ChangeToken.OnChange(
() => env.ContentRootFileProvider.Watch("file.txt"),Computing a hash to prevent extra firing seems reasonable
While computing the extra hash does seem like a good idea, experience has shown that issues like file locking or double save are very common and likely to cause problems, at least on Windows systems.
At my company we've come up with this workaround using delayed invocation. The idea is to wait some time before reading the file and restart the timer if another write is detected.
A singleton anti-pattern is used due to the implementation being an extension method.
However, if such an approach is acceptable, I can do a pull request with a cleaner implementation with a CancellationToken inside the monitor itself.
using System;
using System.Collections.Concurrent;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Options;
namespace ABC.Extensions.Configuration
{
public static class OptionsMonitorExtensions
{
private static readonly ConcurrentDictionary<object, CancellationTokenSource> Tokens
= new ConcurrentDictionary<object, CancellationTokenSource>();
private const int DefaultDelay = 1000;
public static IDisposable OnChangeDelayed<T>(this IOptionsMonitor<T> monitor, Action<T> listener, int delay = DefaultDelay)
{
return monitor.OnChangeDelayed(
(obj, _) => listener(obj),
delay);
}
public static IDisposable OnChangeDelayed<T>(this IOptionsMonitor<T> monitor, Action<T, string> listener, int delay = DefaultDelay)
{
return monitor.OnChange((obj, name) => ChangeHandler(monitor, listener, obj, name));
}
private static void ChangeHandler<T>(IOptionsMonitor<T> monitor, Action<T, string> listener, T obj, string name)
{
var tokenSource = GetCancellationTokenSource(monitor);
var token = tokenSource.Token;
var delay = Task.Delay(DefaultDelay, token);
delay.ContinueWith(
_ => ListenerInvoker(monitor, listener, obj, name),
token
);
}
private static CancellationTokenSource GetCancellationTokenSource<T>(IOptionsMonitor<T> monitor)
{
return Tokens.AddOrUpdate(monitor, CreateTokenSource, ReplaceTokenSource);
}
private static CancellationTokenSource CreateTokenSource(object key)
{
return new CancellationTokenSource();
}
private static CancellationTokenSource ReplaceTokenSource(object key, CancellationTokenSource existing)
{
existing.Cancel();
existing.Dispose();
return new CancellationTokenSource();
}
private static void ListenerInvoker<T>(IOptionsMonitor<T> monitor, Action<T, string> listener, T obj, string name)
{
listener(obj, name);
if (Tokens.TryRemove(monitor, out var tokenSource))
{
tokenSource.Dispose();
}
}
}
}@akamyshanov sure go ahead and file a PR in options and we will take a look
This issue was moved to dotnet/aspnetcore#2542