Ability to disable/bypass caching

[kroymann]

In older versions of the ImageSharp.Web library, it was possible to effectively disable the caching logic by supplying a dummy IImageCache implementation like this:

public class NullImageCache : IImageCache
{
    public IDictionary<string, string> Settings => new Dictionary<string, string>();
    public Task<IImageCacheResolver> GetAsync(string key) => Task.FromResult<IImageCacheResolver>(null);
    public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata) => Task.CompletedTask;
}

We leverage this in our application as we already have a custom caching system applied earlier in the request processing pipeline and thus don't need ImageSharp to add any additional caching. As of version 1.0.3, this technique is no longer possible because the ImageSharpMiddleware was changed to always operate by writing into the cache and then immediately reading back out of the cache again (in commit 2632fc0).

I'd like to add a DisableCaching option to the ImageSharpMiddlewareOptions that gives consumers an officially supported way of opting out of the caching logic. Does this seem like a reasonable feature to add, and if so, would anyone object to me putting together a PR?

[JimBobSquarePants]

I wasn't aware we'd created that regression. Sorry!

Rather than adding an option I'd like to see if we can have another crack at the caching to allow a NullImageCache. It's fair better architecturally that way. If you feel like trying that I'd be very grateful!

[kroymann]

Alright I put together a branch that re-works the ImageSharpMiddleware so that it can return the output stream directly without having to re-read what was just written to the cache. As part of this, I did away with the ReaderWorkers/WriterWorkers pattern entirely and replaced it with the latest async locking code from my product's codebase. NOTE: I left the LRU stuff intact and I tried to minimize unnecessary refactoring in order to make the diff as easy to follow as possible. I also broke the work down into a sequence of step-by-step commits that should help when reviewing it. Take a look at https://github.com/kroymann/ImageSharp.Web/tree/enable_null_cache and let me know what you think. I can also make this into a PR if it's easier to comment on the changes that way.

EDIT: Of course now that I look at the diff in GitHub, I realize that it doesn't ignore whitepace changes (like any proper code diffing tool should allow!), so the changes in indentation make this diff look far more complicated than it actually is 😢

[deanmarcussen]

Did you bench it?

Putting a lock back in would do this (which was one of the reasons it was removed)

if (alreadyCached)
{
    return cachedValue;
}
else
{
// When you lock here, concurrent requests will all have to reprocess the image again, until they have completed
    return UseFancyLockToPreventParallelProcessingOfTheSameImage( () =>
    {
        var image = ProcessImage(); 
        StoreResultInCache(image);
        return image;
    };
}

Before I can really offer any sensible comment here, can you explain a little more about why you want to prevent caching?

If you are doing it earlier in the pipeline, can you not plugin your other cache to the ImageSharp cache?

I don't know if this will be relevant, but we have found the Image Sharp metadata that is cached and used for browser caching very important to keep consistent with what is stored in the Image Sharp cache, or browsers get a invalid cache response when checking if the image has changed.

[kroymann]

I'll explain more about why I want to be able to bypass ImageSharp's caching in second, but first I want to point out that the async locking that I am trying to re-introduce here is actually quite similar to what the code is already doing. It relies on ConcurrentDictionaries and Tasks rather than actual locks, just like the WriterWorkers code. I haven't benchmarked it within ImageSharp but am happy to do so if you can point me at the preferred benchmarking workflow. Also, for what it's worth, these locking primitives have been in heavy use at the core of Rec Room's servers handling a high volume of concurrent traffic for a few years now (obviously that's not a replacement for benchmarking within the context of ImageSharp, but still, it's a decent vote of confidence).

Regarding your question about concurrent requests having to reprocess the image, I actually addressed that in one of my commits (kroymann@f804401). When taking the exclusive writer lock, if the lock wasn't immediately available (which suggests someone else is concurrently processing that image right now), then after obtaining the lock the code re-checks the cache before proceeding to process the image.

Now on the question of why we want to bypass ImageSharp's caching, the TLDR is that we've already got our own layers of caching earlier in the pipeline that provide additional functionality and optimizations. For example, our cache layer stores custom cryptographic content signatures as additional metadata on the entries and those signatures are included in the response via HTTP headers (we use these signatures to prevent hackers from employing HTTP cache injection attacks as a means to get external imagery such as pornography into Rec Room).

[JimBobSquarePants]

I'm curious to see how this runs. The tool used for benchmarking is Crank (though I'll admit I've used it once and usually rely on @deanmarcussen to do it)

https://docs.microsoft.com/en-us/events/dotnetconf-2021/benchmarking-aspnet-applications-with-net-crank

[deanmarcussen]

It might be easier if you created a pr as I haven’t had time to look at the branch at all.
I don’t mind seeing how it performs, but super busy right now. Can take a look early next week.

[marklagendijk]

It might be a good idea to add an example of the NullImageCache to the documentation.
Maybe it would even be nice to add it to the library.

For anyone looking for it, if you have nullability enabled in your project the NullImageCache would be:

public class NullImageCache : IImageCache
{
    public Task<IImageCacheResolver?> GetAsync(string key) => Task.FromResult<IImageCacheResolver?>(null);
    public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata) => Task.CompletedTask;
}

Without nullability:

public class NullImageCache : IImageCache
{
    public Task<IImageCacheResolver> GetAsync(string key) => Task.FromResult<IImageCacheResolver>(null);
    public Task SetAsync(string key, Stream stream, ImageCacheMetadata metadata) => Task.CompletedTask;
}