Issue with multiple requests on different threads against the same image

[svenrog]

Prerequisites

• I have written a descriptive issue title
• I have verified that I am running the latest version of ImageSharp.Web
• I have verified if the problem exist in both DEBUG and RELEASE mode
• I have searched open and closed issues to ensure it has not already been reported

Description

I implemented ImageSharp.Web to leverage resizing on a website. Some page templates on this site feature the same image, in different sizes, in close succession. I noticed that the image that was resolved first in the group, sometimes became reused for the second request. This lead to the site returning the same image for two distinct query strings. I then dug a bit deeper and managed to reproduce the issue in an automated test. Please have a look at this.

Steps to Reproduce

1. Set up a test project with Microsoft.AspNetCore.TestHost, NUnit, NUnit3TestAdapter and SixLabors.ImageSharp.Web installed.
2. Paste this gist into a file ResizerTests.cs.
3. Provide an image to be copied to the output for images/product.jpg (has to be wider than 400px).
4. Note that running the test with client.GetAsync(...) produces unique image sizes, Task.Run(() => client.GetAsync(...)) does not.

System Configuration

• ImageSharp.Web version: 1.0.5
• Other Six Labors packages and versions: ImageSharp 1.0.4
• Environment (Operating system, version and so on): Win10 x64 21H2, 16GB RAM
• .NET Framework version: .NET 5.0

[JimBobSquarePants]

@kroymann Would it be possible for you to comment on this since you built the cache keylock? (Though it could be the LRU cache)

[JimBobSquarePants]

Just ran the tests after completely removing the LRU cache and there were still failures.

[deanmarcussen]

They are useful looking tests, when resolving this, we should put them in the code base permanently, with the other integration tests.

Might be worth dropping the branch back to pre 1.0.4, to see if it's cache/lock related, or not.

(the tests as they are won't work with 1.0.4 because they're relying on a null cache... but they shold plug into the existing test structure ok, I would hope)

[svenrog]

I would like to amend a🤦‍♂️on my part as this might be an issue with the test itself where the string isn't generated directly when Task.Run(() => ) is called and the variable i is then referred later giving it the wrong value.
While I still have duplicate image issues, this test isn't reproducing it correctly.

[deanmarcussen]

Can you fix the test, so it does reproduce the issue?

Or even better, write the test as part of the existing integration test suite?

[kroymann]

The async locking code that I added is focused purely on preventing multiple workers from processing the same exact image request via reader/writer locks, so I doubt that's the culprit. If there's an actual issue here, then it seems like the most likely culprit would be something in the cache code or the LRU cache resolver. I've re-read all of that code a few times over now and can't spot anything obvious. In the absence of a test that repros the issue, I think this is going to be tricky to find.

@svenrog Is it possible that the issue you are seeing is a result of how your app is configured? Are you using a custom implementation of ICacheKey, ICacheHash, or IImageCache? Do you have any caching happening outside the ImageSharp middleware (eg: AspNet Core's Response Caching middleware)?

[svenrog]

Yes it is likely that the problem resides in how (and the order of) other middleware in relation to this plugin. I moved the order of UseResponseCaching to after UseImageSharp and that seems to have had a positive effect. I'll update and close this issue after some additional testing.

[kroymann]

I moved the order of UseResponseCaching to after UseImageSharp and that seems to have had a positive effect.

I'm not sure that's going to work as you intend. UseImageSharp will effectively be the end-of-the-line on the middleware pipeline (assuming it actually handles the request) and the response caching middleware will never actually be invoked. The response caching middleware needs to come first so that it can: (a) have a chance to return a previously cached response before the execution ever reaches ImageSharp, and then (b) have an opportunity to cache a result generated by ImageSharp on the way back out.

Given that you are using the AspNet Core response caching middleware, I have another idea of what the problem might be. By default, the response caching middleware does not care about query string parameters. Requests for the same path with different query string params will be served from the same cache entry. This is obviously not correct for use with ImageSharp.Web and would explain the behavior you are seeing. In my codebase, we have an app builder extension method that looks like this:

public static IApplicationBuilder UseResponseCachingVaryByQuery(this IApplicationBuilder app)
{
    if (app == null)
    {
        throw new ArgumentNullException(nameof(app));
    }

    return app
        .UseResponseCaching()
        .Use((ctx, next) =>
        {
            ctx.Features.Get<IResponseCachingFeature>().VaryByQueryKeys = new[] { "*" };
            return next();
        });
}

And then our middleware pipeline is configured more-or-less like this (plus a bunch of other stuff that's not relevant here):

app.UseResponseCachingVaryByQuery();
app.UseImageSharp();

If you aren't already doing something similar, then that is your problem (and luckily an easy fix!). You can read more about this annoying quirk here: https://docs.microsoft.com/en-us/aspnet/core/performance/caching/middleware?view=aspnetcore-6.0#varybyquerykeys

[svenrog]

Thanks for the in-depth response @kroymann! Your idea is very likely the issue as placing response caching after ImageSharp fixed the issue. This isn't an issue with ImageSharp.

[JimBobSquarePants]

Fantastic news! Thanks everyone. 👍