Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct. #56

Open
codingyourlife opened this issue Nov 2, 2023 · 2 comments

Comments

@codingyourlife
Copy link

codingyourlife commented Nov 2, 2023

We get this error log:
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.

In this line:
return Imagekit.Url(new Transformation()).Path(fileDownloadRequest.ObjectName).Generate();

ChatGPT suggests:

lock (_lockObject)
{
    return Imagekit.Url(new Transformation()).Path(fileDownloadRequest.ObjectName).Generate();
}

But ChatGPT also says:
It seems like you're facing a concurrency issue with the Imagekit.Url method or the underlying collection that it uses. This is common in multi-threaded applications where multiple threads attempt to modify a non-concurrent collection simultaneously.

-> To me it sounds like this is an issue about multithreading inside of your library. The lock statement might have a big performance downside.

I also pasted the content of your ImageKitParams.cs to ChatGPT and asked if it can spot the error. It suggests this part:

if (this.options.ContainsKey(key))
{
    this.options[key] = value;
    if (key == "transformation")
    {
        this.options.Remove("path");
        this.options.Remove("src");
    }
}
else
{
    this.options.Add(key, value);
}

Solution suggestions:

  1. You can make the this.options dictionary thread-safe by using ConcurrentDictionary instead of a regular Dictionary. This would entail modifying all dictionary operations to use methods provided by ConcurrentDictionary like TryAdd, TryRemove, etc.
  2. Another way is to use locking (as I showed in my previous answer) to synchronize access to the this.options dictionary.

Proposed solution in code:
using System.Collections.Concurrent;

namespace Imagekit
{
    public partial class BaseImagekit<T>
    {
        // Assuming this is declared somewhere in your class
        private ConcurrentDictionary<string, object> options = new ConcurrentDictionary<string, object>();

        public T Add(string key, object value)
        {
            options.AddOrUpdate(key, value, (existingKey, existingValue) => 
            {
                if (key == "transformation")
                {
                    options.TryRemove("path", out _);
                    options.TryRemove("src", out _);
                }
                return value;
            });

            return (T)this;
        }

        // ... rest of the class remains unchanged
    }
}

For us the issue is happening pretty much every day in the background.

@codingyourlife
Copy link
Author

@imagekitio can you comment on this?

@samihafidiDLW
Copy link

Hello,

I had the same error because I was trying to reuse the ImageKitClient class multiple times (injecting it once). The implementation of the SDK is not ideal since every time you want to generate a URL, you need to instantiate an ImageKitClient class. This is because the options are stored in a dictionary shared within the same object. Consequently, even if you use a concurrent dictionary, you could end up with options you didn't ask for.

I believe the ImageKitClient should have a static method to generate URLs so it can be reused, but that would require a significant change in the SDK.

I was not using the RestClient since I only needed to generate image URLs in my project. Therefore, I didn't want to create an HttpClient instance for each image URL I wanted to generate. What I did was create a custom implementation of the BaseImagekit (public abstract class). Here is what I did:

public class CustomImageKit : BaseImagekit<CustomImageKit>
{
    public CustomImageKit(string publicKey, string privateKey, string urlEndPoint)
        : base(privateKey, urlEndPoint, "path")
    {
        this.Add("privateKey", privateKey);
        this.Add("publicKey", publicKey);
    }
}

With this approach, you can create a new instance each time, and the dictionary of options will be used for only one image, thereby avoiding the error related to concurrent dictionary usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants