Options 2.0 Iteration 3 - Breaking change edition: Takeover IOptions #179

HaoK · 2017-03-21T22:26:19Z

State of the new world:

Configuring options

Static IConfigureOptions

   services.Configure<MyOptions>(o => o.Do = "Something");
   services.Configure<MyOptions>(configuration); // Binds options to config (also opts into change tracking, for IOptionsSnapshot/Monitor)

Scoped/Transient IConfigureOptions that can depend on services.

   class DbConfigureOptions : IConfigureOptions<MyOptions> {
       public DbConfigureOptions(DbContext db);
       public void Configure(string name, MyOptions options) {
            options.DbValue = ReadSomethingFrom(_db);
       }
   }

   services.Add[Scoped/Transient]<IConfigureOptions<MyOptions>, DbConfigureOptions>();

Consuming options

Static options that never change (no reload, computed once)

   IOptions<MyOptions>.Value

Options that only change between requests (if ever). Options value is cached inside of a scoped service the first time its requested, so even if the underlying cache is updated/invalidated, the snapshot contents will remain unchanged until the next request.

   IOptionsSnapshot<MyOptions>.Value / Get(name)

Direct view into the cache (live updates). Options values are always current, cache invalidations are seen immediately

   IOptionsMonitor<MyOptions>.CurrentValue 
   Also can listen for changes via IOptionsMonitor.OnChange(yourCallback))

Invalidating the cache / updating/reloading options

   // Used by the caches to listen for options changing to trigger invalidation
   IChangeToken IOptionsChangeTokenSource<TOptions>.GetChangeToken() 

   Both IOptionsMonitor(old legacy not pluggable cache) and IOptionsSnapshot (pluggable IOptionsCache) consume the change token sources and do the appropriate cache invalidation.

Example of how to manually trigger a cache invalidation when another

   public class MyOptionsChangeSource : IOptionsChangeTokenSource<MyOptions> {
       IChangeToken GetChangeToken() => _changeToken;
       void Fire() // _changeToken.Invoke() + generate a new token and store in _changeToken
   }
   services.AddSingleton/Scoped/Transient<IOptionsChangeTokenSource<MyOptions>, MyOptionsChangeSource);
   // Note the change source can consume services in the ctor since all lifetimes are supported.

@ajcvickers @divega @davidfowl @glennc

HaoK · 2017-03-21T22:28:18Z

Semantics of name:
name: string.Empty is the default name, and used with the old Configure().
name: null means ignore name, apply to every instance (maps to the new ConfigureAll())
Otherwise name must match for the configure to be applied.

davidfowl · 2017-03-22T02:35:14Z

Oh, wow, breaking changes....

davidfowl · 2017-03-22T02:35:56Z

src/Microsoft.Extensions.Options/IOptionsCache.cs

+    /// Used to cache TOptions instances.
+    /// </summary>
+    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
+    public interface IOptionsCache<TOptions> where TOptions : class


Why is this public? Where would you ever use this?

People were asking for this to be exposed so they can plug in their own caching logic, i.e. using the memory cache to store options and control their own eviction policies, #167

Our default cache will at least have to support the existing reload on change behavior that will refresh any options that are bound to config. I'll be taking a look at that next.

I think that should be fairly straightforward, and is just hooking up the config OnReload changeTokens to call Remove on the IOptionsCache

The hope is that with this new factoring with IOptionsCache, we can obsolete/delete IOptionsMonitor/IOptionsSnapshot, to only have one IOptions to rule them all. (with IOptionsFactory + potentially in a later PR IOptionsValidator as ringwraiths)

HaoK · 2017-03-22T03:21:17Z

Assuming we can make breaking changes like this, this would eliminate the issues around having both old and new interfaces and trying to explain what service works with which interface that the previous iteration had...

MaKCbIMKo · 2017-03-22T16:36:43Z

src/Microsoft.Extensions.Options/IOptions.cs

@@ -7,11 +7,20 @@ namespace Microsoft.Extensions.Options
    /// Used to retreive configured TOptions instances.
    /// </summary>
    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
-    public interface IOptions<out TOptions> where TOptions : class, new()
+    public interface IOptions<TOptions> where TOptions : class, new()


I imagine that we might have a place that requires to have not only the options value, but also the API to operate with options (Get, Add, Remove etc). I also agree that this interface is backward-compatible.

But I think that this interface violates with Interface Segregation principle. If I understand it right, methods (Get, Add, Remove) are required for Auth stuff. But for just retrieving options values isn't required.

Maybe create separated interface IOptionsService with methods (Get, Add, Remove). For basic use-cases inject it in IOptions implementation. For Auth - inject the service itself and operate with options.

Well before, the reason Add was on the main interface was because it needed to do validation using the validator before inserting into the cache. Now that validation is no longer part of this PR, consumers can directly insert what they want into the cache (using the options factory to create it), so for now, its probably fine dropping Add/Remove

Do you think that having Get(name) method in end-user interface is fine?
Should options consumer decide which options is needed? Or DI engine should have an ability and all required information for choosing the right named options ? (and consumer will just consume)

ajcvickers · 2017-03-22T23:48:26Z

src/Microsoft.Extensions.Options/IOptions.cs

@@ -7,11 +7,20 @@ namespace Microsoft.Extensions.Options
    /// Used to retreive configured TOptions instances.
    /// </summary>
    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
-    public interface IOptions<out TOptions> where TOptions : class, new()
+    public interface IOptions<TOptions> where TOptions : class, new()


public interface IOptions where TOptions : class, new() [](start = 4, length = 65)

Have we considered having INamedOptions extend from IOptions and adds the Get method? With the removal of Add and Remove methods, that would mean we would not break current implementors of IOptions while still adding new functionality in 2.0 for those that implement INamedOptions. It also should not result in two parallel sets of APIs, like the previous PR had. And it seems like the two interfaces and their relationship are a reasonable separation of concerns that we might make even if designing this from scratch.

That's closer to the previous iterations where we had a new Options 2.0 interface basically. I definitely prefer not breaking ideally, since already this is a PITA fixing the options usages all over the place. So I thought the concern around potential confusion was related to the usage of the new OptionsFactory/OptionsCache, if we normalizing both IOptions + IOptionsGet to use the new implementation, that sounds good to me as well.

Should we consider a more generic name than INamedOptions since we potentially are looking into adding the validation stuff back soon, I'd hate to rename it right away...

My preference would be to name it something that implies additional logic is being applied on top of IOptions, i.e. IOptionsManager/IOptionsService.

@HaoK IOptions<out T> would remain what it is and INamedOptions<out T>: IOptions<T> would be used to consume named options. If we add validation again, we should add it to both. Then I don't see the advantage of having a new name that implies anything different.

I don't have a problem with splitting Get into its own interface, I just really don't like INamedOptions (which I already used in a previous iteration, it did not grow on me at all...)

Also are we still trying to eliminate IOptionsSnapshot/IOptionsMonitor assuming there are sane workarounds which can be implemented via IOptionsFactory/Cache?

So the behavior difference between IOptions and IOptionsSnapshot is that IOptions will never change, its a cached singleton that never gets updated.

So IOptions might have been a singleton, and aTOptions once returned will never be mutated (by the Options feature at least) but but a new TOptions instance with new values will be returned after the cache is invalidated. Is this correct? If yes, I agree using a method is more appropriate than the Value property. Not sure that determines that the method should always take a name (seems to be an orthogonal question).

Yup, the behavior of the new IOptions.Value implementation that is backed by the IOptionsCache is fully dependent on what's in the cache.

The names stuff is more or less orthogonal, as currently .Value is just returning .Get(string.Empty) name

still think maybe we should consider just adding Get(name) to IOptions, but not break IConfigureOptions as even though we are changing the IOptions interface), there's not many implementations of IOptions today, while implementing your own IConfigureOptions was a major extensibility point which we should avoid breaking

Ah, so you are actually proposing keeping the Value property and adding the method alongside it, correct? Otherwise we would break all consumers of IOptions. I don't like it being a property if it will potentially return a different instance sometimes when invoked, but it may be just me.

Yeah well that's why I was initially proposing we leave IOptions alone entirely and keep the old semantics, and introduce IOptionsManager with the new semantics and no .Value.

If we wanted to be crystal clear about what's old and new, we could leave Options alone entirely, and add all the new stuff in a new Options.Manager package, which would reuse only a few things like IConfigureOptions/Configure

ajcvickers · 2017-03-22T23:59:12Z

Do we need this anymore? Seems like OptionsService can handle this. If we do IOptions and INamedOptions, then maybe this implements IOptions and OptionsService implements INamedOptions. (And we can discuss naming. :-) )

Refers to: src/Microsoft.Extensions.Options/OptionsWrapper.cs:10 in 3cdfe99. [](commit_id = 3cdfe99, deletion_comment = False)

ajcvickers · 2017-03-23T00:03:19Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

            return services;
        }
+
+        public static IServiceCollection ConfigureAll<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions) where TOptions : class


ConfigureAll [](start = 41, length = 12)

What does ConfigurAll do? Also, looks like it will result in null ref exception as currently implemented.

It should just result in a new instance of ConfigureOptions { Name = null, action = action }. Which then eliminates the name check in Configure via (if Name == null || name == Name)

ajcvickers · 2017-03-23T00:04:59Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

@@ -37,7 +39,18 @@ public static IServiceCollection AddOptions(this IServiceCollection services)
        /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
        /// <param name="configureOptions">The action used to configure the options.</param>
        /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
-        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions)
+        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions) where TOptions : class
+            => services.Configure(string.Empty, configureOptions);


string.Empty [](start = 34, length = 12)

Looks like we're saying un-named (legacy) options is just a named options where the name is the empty string. We should consider if this is the right "default name" to use.

Yup, there's a lot of flexibility here, so we can change this as needed.

Also, we also have the option of making legacy ConfigureOptions to mean configuring ALL instances.

ajcvickers · 2017-03-23T00:22:26Z

src/Microsoft.Extensions.Options/ConfigureOptions.cs

        {
            if (options == null)
            {
                throw new ArgumentNullException(nameof(options));
            }

-            Action.Invoke(options);
+            // Null name is used to configure all named options.
+            if (Name == null || name == Name)


Do we need this capability?

ConfigureAll isn't critical, but its a pretty nice to have, otherwise they would have to duplicate common initialization in every name. I'm happy to cut it for now if you want.

ajcvickers · 2017-03-23T00:28:11Z

src/Microsoft.Extensions.Options/OptionsCache.cs

+            {
+                throw new ArgumentNullException(nameof(createOptions));
+            }
+            return _cache.GetOrAdd(name, new Lazy<TOptions>(createOptions)).Value;


new Lazy [](start = 41, length = 18)

Why does this need to be Lazy? GetOrAdd is called with a delegate when you want the TOptions. If the options have not yet been requested, then GetOrAdd will find nothing, and the delegate will be executed to create the options and put them in the dictionary. If the options have already been created, then GetOrAdd will just return them immediately. I'm missing then why Lazy is needed.

I believe this is needed to ensure threadsafety, see https://andrewlock.net/making-getoradd-on-concurrentdictionary-thread-safe-using-lazy/

I looked at how MVC was accomplishing this so I just mimic'd their code, I don't have the full rationale so this is something we can spend some more cycles exploring if needed

That's not really about being thread-safe; it's about the two different patterns for initializing something in a thread-safe way. The one that is generally preferred is that if the object you are initializing is not yet initialized, then you don't try to prevent two things from creating it, but ultimately only one object wins and gets stored. This is usually fine. There are occasions where the initialization code itself cannot run concurrently (usually indicates a different design issue with the initialization code) or it is important that it only ever run one because it has side effects. It does not seem to be that either of these things should be necessary here.

Sure so we are ok with the factory occasionally running more than once, since only one will 'win' and get stored. That said, today we are using LazyInitialzer to guard against this I believe... https://github.com/aspnet/Options/blob/dev/src/Microsoft.Extensions.Options/OptionsCache.cs#L41

Personally I am ok either way. I am cool with using Lazy<T> for this. @ajcvickers do you feel strongly we shouldn't?

I chatted with @ajcvickers offline the other day, he also said he didn't feel strongly and was okay with Lazy to prevent re initialization.

ajcvickers · 2017-03-23T00:36:49Z

src/Microsoft.Extensions.Options/Options.cs

+            {
+                throw new ArgumentNullException(nameof(name));
+            }
+            return _cache.GetOrAdd(name, () => _factory.Create(name));


_cache.GetOrAdd(name, () => _factory.Create(name)); [](start = 19, length = 51)

Initial ideas for async. Separate out the creation and population of options into a new service--something like IOptionsSource. It depends on IOptionsFactory and IOptionsCache. For the normal, non-async, lazy route, OptionsService just depends on this and does what is happening here. But for those who want greater control, the IOptionsSource can be resolved directly and then a Create or Initialize method called that executes the factory, if needed, and stores the result in the cache. On this interface there can be an async overload that will call an async method on the factory. The idea being that the experience and patterns stay the same for normal users, but if you need async. then you can force async resolve explicitly before requesting the options.

IOptionsSource that you mentioned looks the same like IOptions.Value implementation with injected factory. Factory starts working only when we access Value.

The other case if we need Task<TOptions> ValueAsync member. Extend the current interface? or create the inherited (IOptionsAsync) one (for advanced users/cases).

HaoK · 2017-03-23T01:46:32Z

re OptionsWrapper, that existed as a weird hack for non DI scenarios, we still have them a bit (particularly DataProtection uses this for the code paths where they are new'ed up.

I will take a look at those call sites when we apply this change to see if we can do a round of cleanup and fix up those usages, but my guess is we will still need this. Its basically a dumb implementation of IOptions

HaoK · 2017-03-23T01:53:57Z

@ajcvickers So to summarize the set of potential changes for the next iteration:

Lets try to do things non-breaking (exact naming TBD)
Single IOptions2.Get(string name) method, nuke Add/Remove they live only on IOptionsCache
IOptions2 will extend IOptions, and will takeover IOptions implementation, this will let us normalize the old and new behavior however we please. (I'd really prefer not to break IConfigureOptions as that causes breaks in a ton of repos downstream)
Can we leave Async out of this PR, its not needed for Auth, and I don't believe anything we have discussed so far will block some kind of additional IOptionsSource that manipulates the cache. We can have a subsequent PRs focusing soley on Async

HaoK · 2017-03-23T02:02:15Z

test/Microsoft.Extensions.Options.Test/NamedOptionsTests.cs

+        }
+
+        [Fact]
+        public void FactoryCanConfigureAllOptions()


See here for an example of ConfigureAll usage

MaKCbIMKo · 2017-03-23T13:27:00Z

Lets try to do things non-breaking (exact naming TBD)
IOptions2 will extend IOptions, and will takeover IOptions implementation, this will let us normalize the old and new behavior however we please.

I love the idea of extending interfaces (INew : IOld).

I'd really prefer not to break IConfigureOptions as that causes breaks in a ton of repos downstream

Agreed. Moreover, if we will break this interface for named and unnamed interfaces, then it will be much harder to control the order of configurations.

Can we leave Async out of this PR, its not needed for Auth, and I don't believe anything we have discussed so far will block some kind of additional IOptionsSource that manipulates the cache. We can have a subsequent PRs focusing soley on Async

I think we definitely can. Moreover, if it will be implemented by creating new interfaces over the existed, it will be PR without any changes of existing code (e.g. w/o any breaking changes) and will only add the Async stuff. (for now looks like that's real)

HaoK · 2017-03-23T18:31:54Z

Updated:

IOptions.Value + IConfigureOptions<TOptions> unchanged
IOptionsManager.Get(name) (Add/Remove gone)
Added IConfigureNamedOptions<TOptions>
Unification done by default IOptionsFactory which calls the appropriate configure, see OptionsFactory

MaKCbIMKo · 2017-03-23T19:00:48Z

src/Microsoft.Extensions.Options.ConfigurationExtensions/ConfigureFromConfigurationOptions.cs

@@ -19,7 +19,7 @@ public class ConfigureFromConfigurationOptions<TOptions> : ConfigureOptions<TOpt
        /// <param name="name">The name of the options instance.</param>
        /// <param name="config">The IConfiguration instance.</param>
        public ConfigureFromConfigurationOptions(string name, IConfiguration config)


Is parameter name still used? Or can be removed.

MaKCbIMKo · 2017-03-23T19:05:51Z

src/Microsoft.Extensions.Options/OptionsManager.cs

+    /// Implementation of IOptionsFactory.
+    /// </summary>
+    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
+    public class OptionsManager<TOptions> : IOptionsManager<TOptions>, IOptions<TOptions>, IOptionsSnapshot<TOptions> where TOptions : class, new()


IOptionsManager<TOptions> already implements IOptions<TOptions>. Does it require to be duplicated?

Nope, its redundant, good catch I'll remove it

MaKCbIMKo · 2017-03-23T19:14:29Z

...xtensions.Options.ConfigurationExtensions/OptionsConfigurationServiceCollectionExtensions.cs

@@ -19,7 +19,18 @@ public static class OptionsConfigurationServiceCollectionExtensions
        /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
        /// <param name="config">The configuration being bound.</param>
        /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
-        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, IConfiguration config)
+        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, IConfiguration config) where TOptions : class
+            => services.Configure<TOptions>(string.Empty, config);


Probably need to use Options.DefaultName instead of string.Empty.

MaKCbIMKo · 2017-03-23T19:15:54Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

@@ -37,7 +40,18 @@ public static IServiceCollection AddOptions(this IServiceCollection services)
        /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
        /// <param name="configureOptions">The action used to configure the options.</param>
        /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
-        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions)
+        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions) where TOptions : class
+            => services.Configure(string.Empty, configureOptions);


Options.DefaultName instead of string.Empty.

MaKCbIMKo · 2017-03-23T19:16:46Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

            return services;
        }
+
+        public static IServiceCollection ConfigureAll<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions) where TOptions : class
+            => services.Configure(name: null, configureOptions: configureOptions);


Not sure if we still need ConfigureAll method. Probably unnamed Configure does the same.

Unnamed configure only affects Options.DefaultName

Let's imagine the following:

services.Configure<MyOptions>(config1); services.ConfigureAll<MyOptions>(config2); services.Configure<MyOptions>("name", config3);

According the current realization OptionsFactory we will get:

var options1 = Get<MyOptions>(); // will be applied: config1 and config2 var options2 = Get<MyOptions>("name"); // will be applied: config1, config2 and config3

If I'm not missing something, then ConfigureAll does and works the same as unnamed Configure.

Or you're seeing another case?

That's not how it works today. options1 is correct, options2 results in config2 and config3 only.

See

Options/test/Microsoft.Extensions.Options.Test/OptionsSnapshotTest.cs

Line 177 in e80c497

public void NamedSnapshotsConfiguresInRegistrationOrder()

Actually the relevant test is

Options/test/Microsoft.Extensions.Options.Test/OptionsSnapshotTest.cs

Line 196 in e80c497

public void CanConfigureAllDefaultAndNamedOptions()

You're right. My bad.

But IMHO, unnamed Configure and ConfigureAll can be merged into one method.

Nah, Configure has existing meaning today, its configuring the 'default' instance. Giving it a weird sideeffect of configuring all named instances is super weird.

You'd basically be unable to use existing code today which has Configures with named options since every instance would pickup the default configuration.

MaKCbIMKo · 2017-03-23T19:17:49Z

test/Microsoft.Extensions.Options.Test/Microsoft.Extensions.Options.Test.csproj

@@ -19,4 +19,8 @@
    <PackageReference Include="xunit" Version="$(XunitVersion)" />
  </ItemGroup>

+  <ItemGroup>
+    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />


Required service? Am i missing something?

ajcvickers · 2017-03-23T23:10:51Z

I wanted to go over the matrix of scenarios for options being initialized and changed to make sure I understand what people want to do, and see if my understanding maps to this.

In terms of changes, we have:

Options does not change per request (scope)
Options does not change per request but may need to be reset at some point for all subsequent requests
Options can be different for different requests but doesn’t change within a given request
Options can be different for different services within a request (seems unlikely)

In terms of initialization:

Options does not require a request (scope) to be initialized
Options requires a request to initialize

Does this cover all the cases?

Taking initialization first. For options that require a request to be initialized, the IOptionsFactory will need to be scoped. This allows it to depend on other scoped services. For cases where the request is not needed it can be registered as singleton.

For changes:

Options never change => singleton cache. Nothing special needed.
Different for different requests => scoped cache. Nothing special.
Different for all services => no cache/null cache. Nothing special.

So the only case that requires special handling if the case where options are the same for all requests, but might change and that change should be seen by all subsequent requests. This can be handled by invalidating the cache such that the next service that requests the options will create the options again and all subsequent requests will get those new options. So then the only thing needed to make this work nicely is one or more mechanisms to nicely trigger invalidation of the cache. For example, for example, when using configuration binding, the cache should invalidate if the configuration changes.

Did I understand all that correctly? If not, what am I missing.

HaoK · 2017-03-24T05:08:33Z

Yup, I believe that's an accurate summary.

Right now the default OptionsFactory/new Options services are both stateless, so I added them as transient currently rather than scoped (is there a reason to favor scoped instead for stateless implementations?). My main motivation for the new design was to make it straight forward to accomplish all of these (without the old IOptionsMonitor/IOptionsSnapshot interfaces), and as you pointed out, the only 'harder' scenario is the invalidation case, which boils down to implementing the appropriate cache invalidation logic.

HaoK · 2017-03-24T07:33:45Z

One minor clarifications about this point:

Taking initialization first. For options that require a request to be initialized, the IOptionsFactory will need to be scoped. This allows it to depend on other scoped services. For cases where the request is not needed it can be registered as singleton.

So its likely that its not the factory that's depending on scoped services, but rather one of the IConfigureOptions that it uses to build the options that using request services. I'm pretty sure its always a mistake to register the factory as a singleton because of this.

HaoK · 2017-03-24T23:17:25Z

Updated PR with what we discussed (pending approval to takeover IOptions), moved Get(name) + new caching/factory logic to IOptionsSnapshot.

Snapshot continues to capture .Value and Get(name) in a field (snapshot of the cache)
Note: @divega I couldn't change the lifetime of the IConfigureOptions that Configure registers to be scoped, as they are inserting instances, which must have singleton lifetime)
We could update IOptionsMonitor to become the live view of the cache which would let it keep the exact semantic behavior it has today (what you use to have live updates whenever they occur)

HaoK · 2017-03-27T18:58:11Z

Updated the PR implementing configuration reload of options snapshot:

Reused the existing IOptionsChangeTokenSource, added a dependency for Snapshot to register these to invalidate the options cache
With this I think we can delete IOptionsMonitor, there will no longer be any explicit support for getting a 'live' update of the value from the cache, but they could do this by accessing the IOptionsCache. They would have to roll their own callback mechanism for getting notifications when the cache changes.

HaoK · 2017-03-27T19:00:42Z

src/Microsoft.Extensions.Options/OptionsSnapshot.cs

        {
            _cache = cache;
            _factory = factory;
+
+            foreach (var source in changeSources)


This is a little bit weird, basically we are relying on someone using IOptionsSnapshot to 'turn on' change tracking. But I think this is ok, because if no one is listening for changes, then we shouldn't turn on the cache invalidation...

This isn't new though, we had the same issue in the past, someone needed to use the snapshot/monitor to actually run the change token wireup...

divega · 2017-03-28T23:24:13Z

src/Microsoft.Extensions.Options/OptionsSnapshot.cs

+        /// <param name="cache">The cache to use.</param>
+        /// <param name="factory">The factory to use to create options.</param>
+        /// <param name="changeSources">The change token sources used to detect options changes.</param>
+        public OptionsSnapshot(IOptionsCache<TOptions> cache, IOptionsFactory<TOptions> factory, IEnumerable<IOptionsChangeTokenSource<TOptions>> changeSources)


As a consequence of IOptionsFactory<TOptions> being transient, every time we resolve an IOptionsSnapshot<TOptions> we are now creating a new instance of IOptionsFactory<TOptions>, regardless of whether we are going to call Create() on it or not. I don't want to speculate too much about how much this costs because I don't know much about the internals of different DI containers, but it sounds worth measuring the impact of this vs. getting the factory from DI only if needed.

E.g. if the IEnumerable<IConfigureOptions<TOptions>> argument in the constructor of the factory was also re-calculated and instantiated every time, then I believe the cost would be significant.

Yeah I can imagine performance being a big issue when using non singleton IConfigureServices here

I imagine it possibly being an issue if it introduces lots of extra allocations on every request. A possible solution would be to depend on the IServiceProvider and use service location to get the factory lazily. But we should only do this if the issue is real 😄 the code is nicer this way.

HaoK · 2017-03-29T18:20:54Z

@divega @ajcvickers any outstanding concerns preventing me from merging this initial iteration in today?

HaoK · 2017-03-29T18:21:46Z

I'm trying to get in all the new additive changes in this week, so when I'm back from vacation I can do all the breaking changes for auth 2.0 together

HaoK · 2017-03-29T21:10:17Z

Current iteration changes checked in bf76cdb

HaoK added 4 commits March 16, 2017 12:12

OptionsFactory support

40eb483

Iteration 2

5fd3090

Remove concurrent ref

c7bca40

Takeover IOptions

3cdfe99

dnfclas added the cla-already-signed label Mar 21, 2017

HaoK mentioned this pull request Mar 21, 2017

Options 2.0 - Add support for named options, validation, and caching #176

Closed

davidfowl reviewed Mar 22, 2017

View reviewed changes

MaKCbIMKo reviewed Mar 22, 2017

View reviewed changes

ajcvickers reviewed Mar 22, 2017

View reviewed changes

ajcvickers reviewed Mar 23, 2017

View reviewed changes

HaoK commented Mar 23, 2017

View reviewed changes

Split Get from IOptions => IOptionsManager

9600087

Fix config to use named

4ba6b21

MaKCbIMKo reviewed Mar 23, 2017

View reviewed changes

HaoK added 2 commits March 24, 2017 16:05

Retreat to IOptionsSnapshot

563ea44

Add test for default + named + all

145337c

Wireup snapshot to config changes

e80c497

HaoK commented Mar 27, 2017

View reviewed changes

Update lifetimes

3d10f70

divega reviewed Mar 28, 2017

View reviewed changes

HaoK closed this Mar 29, 2017

HaoK mentioned this pull request Apr 13, 2017

The Grand Auth Redesign of 2017 aspnet/Security#1179

Closed

13 tasks

natemcmaster deleted the haok/factory branch November 16, 2017 05:04

Options 2.0 Iteration 3 - Breaking change edition: Takeover IOptions #179

Options 2.0 Iteration 3 - Breaking change edition: Takeover IOptions #179

Conversation

HaoK commented Mar 21, 2017 • edited Loading

State of the new world:

Configuring options

Consuming options

Invalidating the cache / updating/reloading options

HaoK commented Mar 21, 2017

davidfowl commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 22, 2017

MaKCbIMKo Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

divega Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

ajcvickers commented Mar 22, 2017

Choose a reason for hiding this comment

HaoK Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 23, 2017

HaoK commented Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

MaKCbIMKo commented Mar 23, 2017 • edited Loading

HaoK commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Mar 23, 2017

HaoK commented Mar 24, 2017 • edited Loading

HaoK commented Mar 24, 2017 • edited Loading

HaoK commented Mar 24, 2017 • edited Loading

HaoK commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divega Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

divega Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

HaoK commented Mar 21, 2017 •

edited

Loading

MaKCbIMKo Mar 22, 2017 •

edited

Loading

HaoK Mar 23, 2017 •

edited

Loading

divega Mar 24, 2017 •

edited

Loading

HaoK Mar 24, 2017 •

edited

Loading

HaoK Mar 23, 2017 •

edited

Loading

HaoK Mar 23, 2017 •

edited

Loading

HaoK commented Mar 23, 2017 •

edited

Loading

MaKCbIMKo commented Mar 23, 2017 •

edited

Loading

HaoK commented Mar 24, 2017 •

edited

Loading

HaoK commented Mar 24, 2017 •

edited

Loading

HaoK commented Mar 24, 2017 •

edited

Loading

divega Mar 28, 2017 •

edited

Loading

divega Mar 28, 2017 •

edited

Loading

divega Mar 29, 2017 •

edited

Loading