miensol / SimpleConfigSections

An attempt to make defining .net configuration sections a bit simpler
9 stars 5 forks source link

Not Thread safe #6

Open csharpmao opened 8 years ago

csharpmao commented 8 years ago

Hello,

I've got troubles with multiple thread using ConfigSections at the same time.

Problem is in Configuration.WithNamingConvention. If 2 (or more) threads call this method before Configuration.Get is called, then only the last remains, causing exception, as the given type is not expected.

Solution : In NamingConvention :

   [ThreadStatic]
   private static INamingConvention _current;

    public static INamingConvention Current
    {
        get
        {
            return _current ?? (_current = new NamingConvention());

        }...

The rest can stay unchanged.

Thanks for the library, make us gain a lot of time.

Alain

miensol commented 8 years ago

Hi, thanks for the notice. It's true that the Configuration.WithNamingConvention is problematic when used by 2 or more threads or even more than once. While it's not documented this method was supposed to be called only once at the app startup. Your fix seems to be fine - I'll try to apply it over soon.

Out of curiosity you have more than one naming convention?

csharpmao commented 8 years ago

Hi, thanks for the answer.

I have more than one NamingConvention, because my application load multiples plugins, each with it's own configuration section. And each of this plugin has it's own NamingConvention. As I start all the plugins in parallel, I sometimes got some problems.

Thanks,

Alain

miensol commented 8 years ago

I just realised that applying the change you're proposing would break existing library clients. It will work in case you're describing, when each thread set's the [ThreadStatic] variable holding the NamingConvention. However when the NamingConvention is set during app bootstrap from thread A every other thread accessing Configuration would fallback to default NamingConvention. I'll try to figure out what's the easiest way to support your scenario.

csharpmao commented 8 years ago

You're right about the breaking change. Then I propose a override of the Get methods of Configuration and ConfigurationSource : In Configuration.cs

    public static TInterface Get<TInterface>() where TInterface : class
    {
        return new ConfigurationSource().Get<TInterface>();
    }

    public static TInterface Get<TInterface>(INamingConvention namingConvention) where TInterface : class
    {
        return new ConfigurationSource().Get<TInterface>(namingConvention);
    } //...

And in ConfigurationSource

    public TInterface Get<TInterface>() where TInterface : class
    {
        return Get<TInterface>(NamingConvention.Current);
    }

    public TInterface Get<TInterface>(INamingConvention namingConvention) where TInterface : class
    {
        var sectionName = new NamingConvention.CheckForInvalidNamesDecorator(namingConvention).SectionNameByIntefaceOrClassType(typeof(TInterface));            
        var section = ConfigurationManager.GetSection(sectionName);

        if (section == null)
        {
            throw new ConfigurationErrorsException("There is no section named {0}".ToFormat(sectionName));
        }
        return (TInterface)_cachedConfigs.Get(new SectionIdentity(sectionName, typeof(TInterface), 
            (ConfigurationSectionForInterface)section));
    }

I also had to change visibility of NamingConvention.CheckForInvalidNamesDecorator to internal to make it work.

Hopes it works for you,

Thanks,

Alain

miensol commented 8 years ago

This is better 👍 But as you know it will cause the API to behave in a slightly bizarree way - i.e. section name could be taken from different naming convention than other attribute names. I'm trying to change the API so that it's possible to apply NamingConvention on case by case basis.