maartenba / MvcSiteMapProvider

An ASP.NET MVC SiteMapProvider implementation for the ASP.NET MVC framework.
Microsoft Public License
537 stars 220 forks source link

Distributed Caching support #411

Closed ghost1face closed 2 years ago

ghost1face commented 8 years ago

First off, awesome library, its very useful and configurable. On another note, distributed caching isn't supported in this library. I implemented my own provider but then realized a few things:

  1. MicroCache implements a lot of locking which is not needed for distributed cache (not a problem because we can implement IMicroCache)
  2. MicroCache uses LazyLock object and in a distributed environment/cache there is truly no value being stored in cache, just an empty LazyLock object with ICacheDetails. I have referenced exact lines for reference: https://github.com/maartenba/MvcSiteMapProvider/blob/master/src/MvcSiteMapProvider/MvcSiteMapProvider/Caching/MicroCache.cs#L70-L83
  3. WIth the available interface ICacheProvider, it is also tied very strongly to LazyLock which would be easier to implement distributed if used ISiteMap/T instead of LazyLock

https://github.com/maartenba/MvcSiteMapProvider/blob/master/src/MvcSiteMapProvider/MvcSiteMapProvider/Caching/ICacheProvider.cs

I've already forked and started to move things around as needed for my use, just wanted to run that by the community to see what everyone's thoughts are. I can have a code sample available pretty soon as well. I hope I made the issue/suggestions clear, but please comment and let me know if this does sound crazy, I'm sure what you did was intentional, if you could shine some light on what your reasoning was and what we may run into by altering this?

Simplest route we could potentially move the lazy.Get call to prior to adding to cache: https://github.com/maartenba/MvcSiteMapProvider/blob/master/src/MvcSiteMapProvider/MvcSiteMapProvider/Caching/MicroCache.cs#L73

NightOwl888 commented 8 years ago

Thanks for the feedback.

The inspiration for the cache was based on the article linked to in the header of the file. As per the article:

The LazyLock class implements the lazy lock pattern and ensures that a lock is only aquired to populate the value field. This ensures reads are non-blocking. The micro cache implementation effectively stores a lazy lock for each cache key which is responsible for the fetching based on an activator (or value factory).

Although I went through at least 3 different variations of the design before settling on this one, I didn't really consider what would happen if you tried to implement a distributed cache. I agree that this is something that should be addressed.

MicroCache uses LazyLock object and in a distributed environment/cache there is truly no value being stored in cache, just an empty LazyLock object with ICacheDetails. I have referenced exact lines for reference: https://github.com/maartenba/MvcSiteMapProvider/blob/master/src/MvcSiteMapProvider/MvcSiteMapProvider/Caching/MicroCache.cs#L70-L83

The lazy lock is empty when it is stored. Then the Get method is called which either returns the data or fires the method to retrieve it if it is not yet populated. The idea is to release the write lock as early as possible on the cache so other threads can read (potentially other) items in it. That way for a multi-tenant website that uses a SiteMap per tenant won't have to block traffic to all of the SiteMap instances (hence all of the sites) if one of them is in the process of fetching data.

I am not sure there is a way to support both a non-blocking cache and a distributed cache with the same design, but if you have any ideas about how to implement a solution I would be happy to see them.

ghost1face commented 8 years ago

I think the main thing we would need to do is focus on getting the ISiteMap, SiteMap to a simple serializable object which makes this translate to just a hierarchy of nodes. We would need to pull out the injection of services/providers into this object and move that into another service/provider that would be responsible for building the hierarchy.

Those would need to be separated because as it is right now those objects cannot be serialized. Maybe the locking becomes the priority of the ICacheProvider itself if necessary.

I started to go through the code and currently I am working to simplify the SiteMap object to a basic hierarchy and move the other responsibilities. I have nothing solid at the moment but just want to give you a heads up on the direction I am trying to go with this.

I feel this approach would be the starting ground for supporting both in memory and distributed cache.

NightOwl888 commented 8 years ago

One thing to keep in mind is MVC6/DNXCore50 support. One option on the table is just to let MvcSiteMapProvider be phased out with MVC5 and create a new design (either one that supports MVC3+ or one that only supports MVC6).

Since we are basically talking about a new design here (ISiteMap into a Model-like object), would you be interested in contributing to the effort? Some things I would like to see happen:

  1. Make a fluent API that can plug into OWIN/MVC6 startup using extension methods and get rid of the whole internal/external DI approach.
  2. Make it possible to add nodes dynamically rather than having to reload the whole SiteMap to see new ones.
  3. Get rid of the Dynamic Node Provider interface, and make (something like) ISiteMapNodeProvider into a first-class feature.
  4. Remove the XML Sitemap (for search engines) and make it into a separate project/package.
  5. Redesign so that the node-to-action method relationship is always one-to-one, and provide a way for the end user to provide alternate titles, descriptions, route values, etc. when the node is accessed. This will be easier to configure and we won't run into scalability limitations nearly as quickly.
  6. Separate the node data from the node index data so they can be cached separately.
  7. Decouple the nodes from the hierarchy so they don't necessarily have a direct object reference to their parent and children, but have an alternative way to get to the parent and children quickly.
  8. Cache the nodes in segments (possibly 1-1 with ISiteMapNodeProvider) with individual timeouts so the only time the nodes have to be loaded at once is during application startup and so data that is likely to become stale can have a shorter timeout than data that is not.

Joe Audette (the one who opened the MVC6 topic) has a big head start - he has a working implementation on DNX Core 50. There are several ideas he came up with that I like, such as using a ViewModel to adjust the title, description, and URL for each request.

However, his implementation is part of a "do-it-all" framework, which is not an approach I agree with. Also, he has not yet implemented the caching and currently has all of his nodes in trees of TreeNode<T> structures, which I don't think is the best solution for caching (and definitely not thread-safe). Finally, his only options for node configuration are XML and JSON which in my opinion should not be considered the first choice for configuration in a modern app - there should be a DI-friendly configuration API instead.

One thing I have been considering is to use a decorator pattern around the MVC ActionSelector and then building a unique string to identify the action. While the nodes are loading, the ActionSelector could be run to generate the same string. Then the decorator could intercept the action chosen by the ActionSelector and do a fast node match based on the string.

ghost1face commented 8 years ago

I agree on moving forward on an MVC6 approach, but do feel that there are some out there that may need the MVC5 changes as well. Maybe I will put together a separate forked MVC5 version with my suggested changes in the meantime for my usecase and others who may need the same.

I don't mind helping out with this initiative, let's try to coordinate the ideas and get a roadmap planned. I agree with removing the Dynamic Node provider and bringing them together as a first class feature (Dynamic Node provider has been very useful and I'm sure no one would want that completely gone)

NightOwl888 commented 8 years ago

MVC5?? There already is support for MVC 5.