Rationalize IOptionsMonitor for Options 2.0

[HaoK]

With this change: #164 Snapshot no longer will be using the monitor logic at all, we can simplify our surface area by removing all of the monitor/change token related functionality since we no longer need it.

This would be a breaking change for consumers of IOptionsMonitor, but using the IOptionsSnapshot was the mainline scenario for reloadable options so that is unchanged.

[HaoK]

@glennc we should discuss this in triage

[MaKCbIMKo]

May I ask you what you decided to do with OptionsMonitor?

[dazinator]

If you remove the caching / monitoring, this will impact anyone who relies on that functionality. It's quite a common scenario not to want to load settings on every request (i.e cache them) and invalidate the cache only when changes are made imho. See
#174

So if you do remove this, please offer people an alternative route to achieving this goal! I believe asp.net core has a caching library so imho it's not the caching side of the equation that would be a problem (people know how to cache things and invalidate cache with standard caching library), but rather, how to integrate an ICache with the options system.

[HaoK]

Yeah in 2.0 we should definitely look at make caching/lifetime of the options more pluggable in some way, so you'd be able to specify the lifetime of the options, and also do things like evictions/invalidations.

[flatproject]

In V2.0, IOptionsSnapshot will be scoped which means settings will be reloaded on every request. With this change it means that I will not be able to use it anymore in my project as it will hit the database to reload around 800 settings on every request. So, the alternative for me is the IOptionsMonitor which provides the caching capability I'm looking for but it's more awkward to work with (Still haven't managed to reload settings from my custom database configuration provider)

If you are thinking of dropping IOptionsMonitor as this ticket suggests I would say at least make the IOptionsSnapshot to have a pluggable caching module (InMemory or Distributed or etc) with a configurable lifetime policy. Even better with an option to choose which configuration source to invalidate and reload instead of reloading the full configuration when you only want to invalidate and reload a small chunk of it from only a specific source.

[MaKCbIMKo]

I was thinking about how to integrate Options with Caching. We could build our solution based on Caching.Abstraction interfaces.

And probably, we will need to separate Options.Abstractions because Caching already uses Options (circular dependency?).

[HaoK]

Yeah caching will likely be part of the update to the PR #176 today. Options.Abstractions would define the interface, if we wanted to add a Caching + OptionsCache implementation, that would live in a different package.

What I'm thinking for the next iteration of the PR right now:

// Possibly fully async
interface IOptionsManager<TOptions> { // can be transient
  TOptions Value // Same behavior as IOptions.Value just with Validation applied 
  TOptions Get(string name); // Uses cache, creates a new instance if needed and validates via Validator
  void Add(string name, TOptions options); // Adds an externally created options to the cache
  bool Remove(string name); // removes the instance from the cache
}

interface IOptionsCache<TOptions> { // caching behavior plugability point (would be static in memory by default, but can be replaced by scoped ), 
  TOptions Get(string name);
  void Set(string name, TOptions options);
}

interface IOptionsValidator<TOptions> {
    ??? Validate(string name); // extensibility point for how validation actually done,
}

So I'm not sure if we need a snapshot, as that mimics to having a scoped IOptionsCache, but we might if we want to enable some different lifetimes for different options types (IOptionsManager<SingletonOptions> for static ones, and IOptionsSnapshot<RequestOptions>

Thoughts?

[MaKCbIMKo]

Not sure, that IOptionsCache need to be scoped. But I would like to remove unnecessary interfaces.

I was thinking about how to use only one IOptions for injection, but somehow inject into IOptions instance cached and uncached versions of options provider (or Manager/Service). In that way we could configure caching on configuration level. But I need to more consideration and probably try to create a prototype. Still doubt about that.

[HaoK]

@dazinator @flatproject will the new IOptionsService with pluggable IOptionsCache in #176 be sufficient for you to use as a replacement for 1.1's IOptionsSnapshot functionality?

[dazinator]

@HaoK looks ok, so we would basically register our own IOptionsCache<TOptions> . One thing that seemed odd is that OptionsService has an add / remove method for adding / removing options from the cache. Aren't those methods redundant, as people can consume IOptionsCache directly adding or removing from the cache..? ignore me. I can see why it's done this way. Options need to be added via this service because when a new instance is added to the cache, it's setup actions must also be configured to run on that instance. I guess the ability to remove an options instance is just there for completeness.

[dazinator]

I had a bit if a crazy idea that I'll mention just incase there is any value! The setup / configure actions that can mutate an options instance and run in order. What if an action could stop subsequent actions from running. (A bit like short circuiting middleware). If that were possible, a developer could register the actions like so:

1st configure () checks in their cache implementation. If exists then returns cached value and short circuits.
2nd configure() resolves from data source (i.e Json file or database etc) doesn't short circuit.
3rd configure () adds instance to cache.

This is perhaps a bit of a creative workaround, but i think it would enable caching scenarios, without requiring having to implement / register any additional options services.

The idea being, when an options instance is requested, a new options instance is always created, then the actions run in order against that instance. The actions themselves could be set up in such a strategy that as above to achieve caching.

One concern with the PR - as the options classes are mutable, doesn't that introduce the risk that an IOptionsCache will return an instance that could be accessed / mutated from multiple threads?

[MaKCbIMKo]

Hm...I love the idea of having kinda middlewares for configuring an options (something like we do for appBuilder). That would allow to build chains of configurations (with branching by conditions). Moreover, that will make the order of applying these configurations much clear and transparent.

But all of these might be too complicated solution.

[HaoK]

Options already can be accessed and mutated from multiple threads today, as IOptions returns a shared singleton options to every request, and each request is able to manipulate the poco already. The new stuff doesn't change that. We do need to be careful to ensure that only one instance gets into the cache, which is why concurrent dictionary is used to be the gatekeeper. But things should be ok assuming options manipulations are only done via Configures/Validates inside of the Lazy<TOptions> which is running all of the configure/validates inside of ConcurrentDictionary's AddOrGet.

[HaoK]

Also, its possible we will revert any behavior changes we made to IOptionsSnapshot assuming the new IOptionsService covers the per request scenarios we were targeting with Snapshot. No need for the breaking change if we are introducing a new surface area...

[HaoK]

Basically, IOptionsService<ScopedOptions> with a Scoped lifetime IOptionsCache<ScopedOptions> should be equivalent to the changes we made for IOptionsSnapshot in 2.0.

[HaoK]

@dazinator no shortcircuiting :) The idea is all actions will always run, but there's nothing preventing the actions from checking an alreadyInitialized flag and becoming a no-op to roughly give you a short circuiting behavior where first Configure wins. But that sounds like app specific functionality at that point...

[dazinator]

Ok cool! Would be nice to extend the fluent configuration / builder so that we could configure the options cache when setting up options. I had a look but i didn't see such a method yet, but think this would be good for discoverability.

[HaoK]

Not sure if there's any appropriate sugar, an overload of Configure that takes a OptionsCache and service lifetime seems super weird. Its just normal service replacement, services.AddScoped<IOptionsCache<>, MyOptionsCache<>>

[dazinator]

How about if the sugar takes IOptionsCache<TOptions> where TOptions is inferred from Configure<TOptions> and it does the scoped registration for you? So usage would be something like:

services.Configure<TOptions>(foo).UseCache<MyCache>();

Just thinking about discoverability. IOptionsCache<> would be registered as scoped, but I don't think that's a problem as any custom implementation could have other services injected from other scopes if required, or use a static backing field in their implementation (i.e concurrent dictionary) for the cache state?

[MaKCbIMKo]

@HaoK @dazinator - I also was thinking about pretty the same API for registering cache for options:

services.Configure<TOptions>(foo).UseCache(); // use default implementation of cache
services.Configure<TOptions>(foo).UseCache<MyCache>(); // use custom implementation of cache

and probably:

// in case if can apply cache globally for all options
services.Configure<TOptions>(foo).UseNoCache();  // it will disable cache for options that we need

But still thinking about lifetime...

[dazinator]

RE: lifetime.. Could just always register IOptionsCache as scoped lifetime. Then any imementation could have any required services injected from other scopes if needed. For example the default implementation could just use a static backing field (concurrent dictionary) or have a separate service injected to manage the cache state, which would be registered as singleton?

[flatproject]

@HaoK #176 is definitely sufficient for what i'm doing and a good replacement for V1.1 IOptionsSnapshot functionality.

I also like @dazinator @MaKCbIMKo idea on how to register the cache for options and be able to provide a custom cache implementation as well.

Is there going to be an easy(ier) way to invalidate a specific configuration source cache and reload it?

[HaoK]

@flatproject I'm not sure how much easier it will be. We were hoping that scoped options would be enough for the majority of those cases. With the more advanced singleton cache with invalidation triggered config reload still possible, but not be the mainline path.

[HaoK]

Whelp, not much chance of removing monitor now that logging is going to use it potentially...

https://github.com/aspnet/Logging/pull/626/files