ReloadOnChange stops after error in Load

[FlorianRainer]

Version 1.0.0
Net: 4.6

I use the ConfigurationBuilder in a simple console application, nothing special
.AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)

I use a worker to write my config value each second to the output.
Reloading works without problems, but if i save the config file with an error, even if i correct it later it stopps to reload.

but sometimes it happens even if don't save with errors, if i save to fast twice, or at a random number of config changes, maybe ~10.

the problem can be the same, by reading a partialy saved file.
i get no visible exception, but the reloadOnChange stopps.

This is a crucial issue for me cause im using this inside a windows service, and if the config dont change i need to restart the entire service, and its hard to see if changes happen or not.

[HaoK]

Might be related to #476

[divega]

@HaoK you can decide to close this as duplicate if you think we have enough information in other issues.

[FlorianRainer]

@HaoK its not throwing any exception in the application, maybe its the exception in the FileSystemWatcher Callback, but not sure, i don't see it in VS.

But there remains the problem when i change the file about 10 times without error the reload mechanism stopps as well. maybe there is a fileread exception because of a to early read, but ist hard to know without debugging.

but i saw there are some issues with the Reload Token as well, it could be better to refactor the way how the Reload Mechanism will be triggered, in a more stable way.

[HaoK]

General problem, is that exceptions in OnLoad will cause the OnChange chain to be broken so no more subsequent ReloadOnChange notifications will happen. That explains why any parse errors will cause no more change notifications.

cc @divega

[BrennanConroy]

👀

[HaoK]

@BrennanConroy ran into this due to File.WriteAllText will trigger reload before the file has been completely written, this almost certainly will break reload on change entirely as it currently stands...

At a minimum we need to make the change monitoring be able to handle exceptions so they don't blow up future exceptions, need to wrap the call to Load in a try/catch is a pretty simple fix for that

[divega]

that exceptions in OnLoad will cause the OnChange chain to be broken so no more subsequent ReloadOnChange notifications will happen

I was going to suggest a try/finally block. I saw your PR has try/catch that swallows the exception instead. In principle I would still prefer try/finally if it helps, but I am not sure I can articulate a convincing reason in this particular case 😄

File.WriteAllText will trigger reload before the file has been completely written

Should we implement a back-off strategy as a heuristic to give file changes the chance to be completed before we react? Not sure if blocking fow a while here here would have any bad side effects.

[HaoK]

Do you strongly prefer try/finally over try/catch? In this case I lean towards swallowing since there is no way they can catch the exception right now (other than putting the try/catch inside of the delegate being passed to OnChange.

I'm not sure I understand what you are proposing in regards to the WriteAllText, since we already 'miss' any notifications that would occur during processing of the prior callback, adding MORE opportunity to miss notifications doesn't seem ideal.

[FlorianRainer]

i have noticed multiple callbacks from FileSystemWatcher. Maybe the cause of your problem is this behaviour. Maybe the first callback is already called while the file is not completely written, but the last one would match your needs.
I have not tested this in any case but maybe it could be a hint.

I have resolved the problem by adding a small delay to the OnLoadcall, i start a Timer on OnChangecallback and restart the timer on each following callback, until a second is passed with no more callback.
then the timer triggeres the OnLoadevent.

OFC its delayed but this is not critical for my usecase, its more critical to have multiple callbacks in a short period of time.

[HaoK]

Yes that's exactly what uncovered the root issue, since some of the config providers will throw if the file isn't valid, which prevents OnChange from reregistering a change token which breaks future notifications from being received.

So the lowest level bug, is OnChange needs to be able to handle the consumer blowing up with an exception. I believe once that's fixed, the extra 'too early' callbacks into the config system should be fine (and safely ignored, but this will require a few other minor fixes in the ConfigurationProvider base code as well to work cleanly today).

[divega]

Do you strongly prefer try/finally over try/catch? In this case I lean towards swallowing since there is no way they can catch the exception right now (other than putting the try/catch inside of the delegate being passed to OnChange.

Moving discussion to the PR at https://github.com/aspnet/Common/pull/147/files#r78456599.

I'm not sure I understand what you are proposing in regards to the WriteAllText, since we already 'miss' any notifications that would occur during processing of the prior callback, adding MORE opportunity to miss notifications doesn't seem ideal.

Adding a back-off period is just an idea for making the feature more robust without trying to make it deterministic. E.g. if a change happened and the notification didn't get triggered then the feature would obviously be broken. But if the notification got triggered 10 milliseconds after the change and "collected" any additional changes that happened in the meanwhile, I believe the feature could still probably work better than if it tried to raise a separate notification for every single change.