CODEWITHSUNDEEP
c#multithreadingcachingasynchronous
amateur
amateur

Reputation: 44605

locking object in async operation

I have the following code that I want to achieve the following with.

  1. Check if a value is in cache
  2. If in cache, get the value from it and proceed
  3. If not in cache, perform the logic to enter it in cache but do this async as the operation to do such may take a long period of time and I dont want to hold up the user

As you will see in my code I place a lock on the cache in the async thread. Is my setup below thread safe? And by placing the lock will this mean that the cache will not be accessible for other threads to read from cache while the async operation takes place. I do not want a circumstance where the cache is locked in an async thread preventing other requests from accessing it.

There is also a chance that the same request may be called by several threads hence the lock.

Any recommendations as how I could improve the code would be great.

// Check if the value is in cache
        if (!this.Cache.Contains(key))
        {
            // Perform processing of files async in another thread so rendering is not slowed down
            ThreadPool.QueueUserWorkItem(delegate
            {
                lock (this.Cache)
                {
                    if (!this.Cache.Contains(key))
                    {
                        // Perform the operation to get value for cache here
                        var cacheValue = operation();

                        this.Cache.Add(key, cacheValue);
                    }
                }
            });

            return "local value";
        }
        else
        {
            // Return the string from cache as they are present there
            return this.Cache.GetFilename(key);
        }

Note: this.Cache represents a cache object.

The application is a web application on .net 3.5.

Upvotes: 4

Views: 4886

Answers (3)

dtb
dtb

Reputation: 217293

There are several problems with your code. Problems include: calling Cache.Contains outside a lock while other threads may be modifying the collection; invoking operation within a lock which may cause deadlocks; etc.

Here's a thread-safe implementation of a cache that satisfies all your requirements:

class Cache<TKey, TValue>
{
    private readonly ConcurrentDictionary<TKey, Task<TValue>> items;

    public Cache()
    {
        this.items = new ConcurrentDictionary<TKey, Task<TValue>>();
    }

    public Task<TValue> GetAsync(TKey key, Func<TKey, TValue> valueFactory)
    {
        return this.items.GetOrAdd(key,
            k => Task.Factory.StartNew<TValue>(() => valueFactory(k)));
    }
}

The GetAsync method works as follows: First it checks if there is a Task in the items dictionary for the given key. If there is no such Task, it runs valueFactory asynchronously on the ThreadPool and stores the Task object that represents the pending asynchronous operation in the dictionary. Code calling GetAsync can wait for the Task to finish, which will return the value calculated by valueFactory. This all happens in an asynchronous, non-blocking, thread-safe manner.

Example usage:

var cache = new Cache<string, int>();

Task<int> task = cache.GetAsync("Hello World", s => s.Length);

// ... do something else ...

task.Wait();
Console.WriteLine(task.Result);

Upvotes: 1

Alex Shtoff
Alex Shtoff

Reputation: 2640

How about changing the delegate to look like this:

var cacheValue = operation();
lock (this.Cache)
            {
                if (!this.Cache.Contains(key))
                {
                    // Perform the operation to get value for cache here

                    this.Cache.Add(key, cacheValue);
                }
            }

This kind of coding locks the dictionary for a very short time. You can also try using ConcurrentDictionary that mostly doesn't to any locking at all.

Alex.

Upvotes: 1

Robert Wagner
Robert Wagner

Reputation: 17793

Looks like a standard solution, except for the retrieval in the background thread. It will be thread safe as long as all other bits of the code that use the cache also take out a lock on the same cache reference before modifying it.

From your code, other threads will still be able to read from the cache (or write to it if they don't take out a lock(). The code will only block at the point a lock() statement is encountered.

Does the return "local value" make sense? Would you not need to retrieve the item in that function anyway in the case of a cache miss?

Upvotes: 0

Related Questions