TL;DR: I have a cache that depends on a timer. The cache is a singleton, and the timer must be a transient (otherwise, different components that need a timer would share the same timer). This is a captive dependency, which is a Bad Sign™. What's wrong with this design, then? It seems logical to me that a singleton component A should be able to make use of another component Z that must be instantiated uniquely for each component A, B, C, ... that depends on it.
I'm developing an ASP.NET web API for serving aggregated sales data. In my backend, I have an OrderRepository decorated by a CachingOrderRepository. I register both using SimpleInjector. It seems logical to me that both of these should be registered as singletons – the cache, at least, has to be (otherwise AFAIK a new/empty cache would be instantiated for each web request, defeating the purpose of a cache).
CachingOrderRepository works by silently updating itself each night (keeping all orders in memory), and thus depends on ITimer (a mockable wrapper for System.Timers.Timer). I have no problems with long-lived timers (this particular one will effectively be a singleton, by design), but it needs to be registered as a transient dependency because each component that needs a timer needs a unique instance.
However, since the cache is registered as a singleton, and the timer is registered as a transient, this is a captive dependency and gives a lifestyle mismatch warning in SimpleInjector, which they say should not be ignored:
When to Ignore Warnings
Do not ignore these warnings. False positives for this warning are rare and even when they occur, the registration or the application design can always be changed in a way that the warning disappears.
I expect the error is with my design, and that this is not one of those rare false positives. My question then is this: What, specifically, is wrong with my design?
I realize I can circumvent the warning by making the cache depend upon an abstract factory for ITimer, but that seems to be just that – a means of circumventing the warning by moving the construction of ITimer instances from SimpleInjector to an explicit factory, costing me another layer of abstraction that needs to be mocked in order to unit test CachingOrderRepository (I need to inject a factory mock that returns a timer mock). If you suggest to use an abstract factory, please explain why this is more than just a way to circumvent the warning (in this particular case, it would let the parent components control the disposal of the timer, but in general, the component returned by the factory might not be a disposable so this would not be a problem).
(Note: This is not a question of what is generally wrong with captive dependencies, which have good answers other places. It is a question about what is wrong with the specific design mentioned above. However, if I appear to have misunderstood something about captive dependencies, feel free to clarify.)
Update: As requested, here's the implementations of the cache and the timer:
public class CachingOrderRepository : IOrderRepository
{
private readonly IOrderRepository repository;
private ITimer timer;
private Order[] cachedOrders;
private bool isPrimed;
public CachingEntityReader(IOrderRepository repository, ITimer timer)
{
this.repository = repository;
this.timer = timer;
this.StartTimer();
}
private void StartTimer()
{
this.timer.Elapsed += this.UpdateCache;
this.timer.Interval = TimeSpan.FromHours(24);
this.timer.Start();
}
private void UpdateCache()
{
Order[] orders = this.repository.GetAll().ToArray();
this.cachedOrders = orders;
this.isPrimed = true;
}
public IEnumerable<Order> GetAllValid()
{
while (!this.isPrimed)
{
Task.Delay(100);
}
return this.cachedOrders;
}
}
public class TimedEventRaiser : ITimedEventRaiser, IDisposable
{
private readonly Timer timer;
public TimedEventRaiser()
{
this.timer = new Timer();
this.timer.Elapsed += (_, __) => this.Elapsed?.Invoke();
}
public event Action Elapsed;
public TimeSpan Interval
{
get => TimeSpan.FromMilliseconds(this.timer.Interval);
set => this.timer.Interval = value.TotalMilliseconds;
}
public void Start()
{
this.timer.Start();
}
public void Stop()
{
this.timer.Stop();
}
public void Dispose()
{
this.timer.Dispose();
}
}
but it needs to be registered as a transient dependency because each component that needs a timer needs a unique instance.
The behavior that you are describing is actually a different lifestyle, which Simple Injector has no support for out of the box, namely: Instance Per Dependency. Such lifestyle is easily created, but typically not advised, because, as described, a design can typically be adjusted to make the use of stateful dependencies unneeded.
So it's not that it's bad per se to have an instance per dependency, but in general, the use of stateless and immutable components leads to cleaner, simpler and more maintainable code.
For instance, in your case your code will benefit from extracting the responsibility of caching from the decorator itself. This will simplify the decorator and allows a caching implementation to become a Singleton as well.
The following example shows a simplified version of your CachingOrderRepository:
public class CachingOrderRepository : IOrderRepository
{
private readonly IOrderRepository repository;
private readonly ICache cache;
private readonly ITimeProvider time;
public CachingOrderRepository(
IOrderRepository repository, ICache cache, ITimeProvider time)
{
this.repository = repository;
this.cache = cache;
this.time = time;
}
public IEnumerable<Order> GetAllValid() =>
this.cache.GetOrAdd(
key: "AllValidOrders",
cacheDuration: this.TillMidnight,
valueFactory: () => this.repository.GetAllValid().ToList().AsReadOnly());
private TimeSpan TillMidnight => this.Tomorrow - this.time.Now;
private DateTime Tomorrow => this.time.Now.Date.AddHours(24);
}
This CachingOrderRepository makes use of an ICache abstraction that looks like this:
public interface ICache
{
T GetOrAdd<T>(string key, TimeSpan cacheDuration, Func<T> valueFactory);
}
An adapter implementation for a caching library like the ASP.NET cache is easily built on top of this abstraction. As an example, here is an adapter for MemoryCache:
public sealed class MemoryCacheCacheAdapter : ICache
{
private static readonly ObjectCache Cache = MemoryCache.Default;
public T GetOrAdd<T>(string key, TimeSpan cacheDuration, Func<T> valueFactory)
{
object value = Cache[key];
if (value == null)
{
value = valueFactory();
Cache.Add(key, value, new CacheItemPolicy
{
AbsoluteExpiration = DateTimeOffset.UtcNow + cacheDuration
});
}
return (T)value;
}
}
UPDATE
Do note that multi-threading and locking are really hard problems (as your Gist samples show), so if you require your solution to automatically update in the background, I would suggest having a piece of infrastructure, external to both the cache and the decorator, trigger the update. This allows that piece of infrastructure to go through the pipeline and trigger the update as a normal user would. For instance:
// Start the operation 2 seconds after start-up.
Timer timer = new Timer(TimeSpan.FromSeconds(2).TotalMilliseconds);
timer.Elapsed += (_, __) =>
{
try
{
// Wrap operation in the appropriate scope (if required)
using (ThreadScopedLifestyle.BeginScope(container))
{
var repo = container.GetInstance<IOrderRepository>();
// This will trigger a cache reload in case the cache has timed out.
repo.GetAllValid();
}
}
catch (Exception ex)
{
// TODO: Log exception
}
// Will run again at 01:00 AM tomorrow to refresh the cache
timer.Interval = DateTime.Today.AddHours(25);
};
timer.Start();
You can add this code at application startup. This code will ensure that 2 seconds after start-up, the cache is loaded in the background, and it will do so again at 01:00 AM the next day.
This is much easier to implement, because the timer is guaranteed to be unique, so you don't have to worry about multiple callers triggering this at the same time, while your cache implementation can still apply a global lock while updating, which is much simpler.
Downside of this approach is that you can't update with concurrent readers, but this might not be a problem to you, especially when running the update during the night.