Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Fix reload behaviors + Add OnLoadError event for file based config providers #497

Closed
wants to merge 6 commits into from

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Aug 23, 2016

Fixes #489

No sugar currently:

                Exception jsonError = null;
                Action<Exception> jsonLoadError = ex => jsonError = ex;
                new ConfigurationBuilder()
                    .Add(new JsonConfigurationSource { Path = "error.json", OnLoadError = jsonLoadError })
                    .Build();

This at least enables some way to see what failures are occuring during load/reload

@divega

@HaoK
Copy link
Member Author

HaoK commented Aug 25, 2016

@divega ping for review

@@ -32,6 +33,11 @@ public abstract class FileConfigurationSource : IConfigurationSource
public bool ReloadOnChange { get; set; }

/// <summary>
/// Will be called if an uncaught exception occurs in FileConfigurationProvider.Load.
/// </summary>
public Action<Exception> OnLoadError { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

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

We could consider making this a Func that returns bool to control whether we want to rethrow the exception. That might be a nice quality of life improvement, but not sure how useful it is always...

Copy link

@divega divega Aug 26, 2016

Choose a reason for hiding this comment

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

I like the idea, although not sure the meaning of returning true vs. false is clear. Maybe passing a "load error context" object that contains the exception, the sender (provider) and a bool to decide the outcome?

@HaoK
Copy link
Member Author

HaoK commented Aug 31, 2016

@divega updated adding the FileLoadErrorContext and renamed LoadError => LoadException as we discussed.

@HaoK
Copy link
Member Author

HaoK commented Sep 9, 2016

SetFileLoadExceptionHandler

@HaoK
Copy link
Member Author

HaoK commented Sep 9, 2016

@divega I don't think our CreateInstance/Build hack is going to work, as it is a breaking change.

Lets say they derived from our current sources today and in their Build(), they all base.Build(), they will now get a NotImplemented exception since they didn't implement CreateInstance.

I think we should just introduce a new method in FileConfigurationSource and call it in each of the Builds, its not the end of the world...

   // in FileConfigurationSource
   public void EnsureDefaults(IConfigurationBuilder builder) {
            FileProvider = FileProvider ?? builder.GetFileProvider();
            OnLoadException = OnLoadException ?? builder.GetFileLoadExceptionHandler();
   }

        // boilerplate builds in all FileProviders
        public override IConfigurationProvider Build(IConfigurationBuilder builder)
        {
            EnsureDefaults();
            return new JsonConfigurationProvider(this);
        }

@HaoK
Copy link
Member Author

HaoK commented Sep 9, 2016

Updated with new IConfigurationBuilder.Get/SetFileLoadExceptionHandler sugar

@divega
Copy link

divega commented Sep 9, 2016

Lets say they derived from our current sources today and in their Build(), they all base.Build(),

They can't. You get a compiler error if you try to call base.Build() if Build() is abstract on the base.

@divega
Copy link

divega commented Sep 10, 2016

... anyway, I am not against what you did here. My only concern right now is the inconsistent naming between the Set/Get methods and the property on the file sources.

@divega
Copy link

divega commented Sep 10, 2016

I think we should consider renaming OnLoadException to LoadExceptionHandler or FileLoadExceptionHandler.

@HaoK HaoK changed the title Add OnLoadError event for file based config providers Fix reload behaviors + Add OnLoadError event for file based config providers Sep 15, 2016
@HaoK
Copy link
Member Author

HaoK commented Sep 15, 2016

@divega @glennc updated PR with fixes to make reload behavior more reliable:

  • we now clear the configuration keys at the start of reload (old keys will now be dropped)
  • we add a configurable delay (FileConfigurationSource.ReloadDelay) before starting reload (default 250 ms)
  • Reenabled all linux tests, lowered sleeps to 1100 ms in the tests(from 2000), since we should be more reliable with the build in delay as well

@HaoK
Copy link
Member Author

HaoK commented Sep 15, 2016

This PR now also fixes #486

@HaoK
Copy link
Member Author

HaoK commented Sep 21, 2016

36ec5b0

@HaoK HaoK closed this Sep 21, 2016
@natemcmaster natemcmaster deleted the haok/8-23loaderr branch November 6, 2018 22:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants