ImageSharp.Web.Caching.AsyncKeyLock.GetDoorman: System.InvalidOperationException

[dotli]

Description

System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

Steps to Reproduce

   at System.Collections.Generic.Dictionary`2.FindEntry(TKey key)
   at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
   at SixLabors.ImageSharp.Web.Caching.AsyncKeyLock.GetDoorman(String key) in ImageSharp.Web\Caching\AsyncKeyLock.cs:line 76
   at SixLabors.ImageSharp.Web.Caching.AsyncKeyLock.ReaderLockAsync(String key) in ImageSharp.Web\Caching\AsyncKeyLock.cs:line 47
   at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext context) in ImageSharp.Web\Middleware\ImageSharpMiddleware.cs:line 187

System Configuration

• ImageSharp.Web version: master
• Other SixLabors packages and versions:
  SixLabors.ImageSharp 1.0.0-dev002697
• Environment (Operating system, version and so on):
  Windows Server 2012 Standard
• .NET Framework version:
  .Net Core 2.1

[JimBobSquarePants]

Hey @dotli

Thanks for filling out the template.

Can you please provide steps to reproduce the issue. The stacktrace is useful but I don't know how to reproduce the problem.

[JimBobSquarePants]

@kroymann @sebastienros

Since you both helped write the locking code I thought it was best to get a deeper understanding here.

We're using SpinLock but from my research it's not a good fit performance-wise for our Write method as we would be holding on the the lock potentially for seconds as opposed to the recommended microsecond upper limit.

I'm also able to cause the internal Dictionary to throw An item with the same key has already been addedwhen writing using the following code which suggests we are not locking properly:

private async Task AsyncLockCanLockByKey()
{
    bool zeroEntered = false;
    bool entered = false;
    int index = 0;
    Task[] tasks = Enumerable.Range(0, 5).Select(i => Task.Run(async () =>
    {
        using (await AsyncLock.WriterLockAsync(AsyncKey).ConfigureAwait(false))
        {
            if (i == 0)
            {
                entered = true;
                zeroEntered = true;
                await Task.Delay(3000).ConfigureAwait(false);
                entered = false;
            }
            else if (zeroEntered)
            {
                Assert.False(entered);
            }

            index++;
        }
    })).ToArray();

    await Task.WhenAll(tasks);
    Assert.Equal(5, index);
}

Any ideas?

References
http://www.albahari.com/threading/part5.aspx#_SpinLock_and_SpinWait
https://docs.microsoft.com/en-us/dotnet/standard/threading/how-to-use-spinlock-for-low-level-synchronization

[JimBobSquarePants]

See also. #64

I managed to replicate this at one point but not sure how as I was hacking together the above test.

[omariom]

SpinLock is a mutable struct. Calling methods on a readonly location of an instance of a struct makes a copy. So mutation is lost.

https://github.com/SixLabors/ImageSharp.Web/blob/master/src/ImageSharp.Web/Caching/AsyncKeyLock.cs#L30

[Drawaes]

dotnet/roslyn#17310

[kroymann]

Agh, I've been burned by this exact problem enough times now that you'd think I would have learned by now. @omariom and @Drawaes are correct. Dropping the readonly modifier from the SpinLock should fix this.

That said, I think this AsyncKeyLock system could potentially be improved a bit. As currently implemented it has the potential to become a bottleneck under heavy concurrent access. And while that may not be a huge problem for ImageSharp.Web given the realistic throughput of a server running something as heavy-weight as image processing, I've seen this same exact AsyncKeyLock code being copied into other codebases where really high concurrency is a potential concern, so it's probably worth making this as solid as possible.

A few months ago I overhauled the locking logic in our own codebase and was able to replace the SpinLock+Dictionary logic with a proper ConcurrentDictionary. It might be worth trying to port that into ImageSharp.Web. I'm currently in Costa Rica on vacation with my family (yeah I know, I really shouldn't be replying to GitHub issues right now...), but when I get back I'll see if I can find some time to port it over and open up a PR to discuss the merits of it.

[JimBobSquarePants]

Thanks for the input guys, all very helpful! ❤️

@kroymann Tut, tut. get back to your holiday! We'll chat when you're back, keen to hear your ideas.