Lock around lambda event handler

Question

I need to synchronize a sequence of operations that contains an asynchronous part. The method looks into an image cache and returns the image if it's there (invokes a callback in reality). Otherwise it has to download it from the server. The download operation is asynchronous and fires an event on completion.

This is the (simplified) code.

private Dictionary<string, Bitmap> Cache;

public void GetImage(string fileName, Action<Bitmap> onGetImage)
{
    if (Cache.ContainsKey(fileName))
    {
        onGetImage(Cache[fileName]);
    }
    else
    {
        var server = new Server();
        server.ImageDownloaded += server_ImageDownloaded;
        server.DownloadImageAsync(fileName, onGetImage); // last arg is just passed to the handler
    }
}

private void server_ImageDownloaded(object sender, ImageDownloadedEventArgs e)
{
    Cache.Add(e.Bitmap, e.Name);
    var onGetImage = (Action<Bitmap>)e.UserState;
    onGetImage(e.Bitmap);
}

The problem: if two threads call GetImage almost at the same time, they will both call the server and try to add the same image to the cache. What I should do is create lock at the beginning of GetImage and release it at the end of the server_ImageDownloaded handler.

Obviously this is not doable with the lock construct and it would not make sense, because it would be difficult to ensure that the lock is realeased in any case.

Now what I thought I could do is use a lambda instead of the event handler. This way I can put a lock around the whole section:

I have to lock the Cache dictionary at the beginning of the DownloadImage method and release it only at the end of the ImageDownloaded event handler.

private Dictionary<string, Bitmap> Cache;

public void GetImage(string fileName, Action<Bitmap> onGetImage)
{
    lock(Cache)
    {
        if (Cache.ContainsKey(fileName))
        {
            onGetImage(Cache[fileName]);
        }
        else
        {
            var server = new Server();
            server.ImageDownloaded += (s, e) =>
            {
                Cache.Add(e.Bitmap, e.Name);
                onGetImage(e.Bitmap);
            }
            server.DownloadImageAsync(fileName, onGetImage); // last arg is just passed to the handler
        }
    }
}

Is this safe? Or the lock is immediately released after execution of GetImage, leaving the lambda expression unlocked?

Is there a better approach to solve this problem?

---

SOLUTION

In the end the solution was a bit of a mix of all the answers and comments, unfortunately I cannot mark-as-answer all of them. So here is my final code (removed some null checks/error cases/etc. for clarity).

private readonly object ImageCacheLock = new object();
private Dictionary<Guid, BitmapImage> ImageCache { get; set; }
private Dictionary<Guid, List<Action<BitmapImage>>> PendingHandlers { get; set; }

public void GetImage(Guid imageId, Action<BitmapImage> onDownloadCompleted)
{
    lock (ImageCacheLock)
    {
        if (ImageCache.ContainsKey(imageId))
        {
            // The image is already cached, we can just grab it and invoke our callback.
            var cachedImage = ImageCache[imageId];
            onDownloadCompleted(cachedImage);
        }
        else if (PendingHandlers.ContainsKey(imageId))
        {
            // Someone already started a download for this image: we just add our callback to the queue.
            PendingHandlers[imageId].Add(onDownloadCompleted);
        }
        else
        {
            // The image is not cached and nobody is downloading it: we add our callback and start the download.
            PendingHandlers.Add(imageId, new List<Action<BitmapImage>>() { onDownloadCompleted });
            var server = new Server();
            server.DownloadImageCompleted += DownloadCompleted;
            server.DownloadImageAsync(imageId);
        }
    }
}

private void DownloadCompleted(object sender, ImageDownloadCompletedEventArgs e)
{
    List<Action<BitmapImage>> handlersToExecute = null;
    BitmapImage downloadedImage = null;

    lock (ImageCacheLock)
    {
        if (e.Error != null)
        {
            // ...
        }
        else
        {
            // ...
            ImageCache.Add(e.imageId, e.bitmap);
            downloadedImage = e.bitmap;
        }

        // Gets a reference to the callbacks that are waiting for this image and removes them from the waiting queue.
        handlersToExecute = PendingHandlers[imageId];
        PendingHandlers.Remove(imageId);
    }

    // If the download was successful, executes all the callbacks that were waiting for this image.
    if (downloadedImage != null)
    {
        foreach (var handler in handlersToExecute)
            handler(downloadedImage);
    }
}

Answer 1

You have another potential problem here. This code:

if (Cache.ContainsKey(fileName))
{
    onGetImage(Cache[fileName]);
}

If some other thread removes the image from the cache after your call to ContainsKey but before the next line is executed, it's going to crash.

If you're using Dictionary in a multi-threaded context where concurrent threads can be reading and writing, then you need to protect every access with a lock of some kind. lock is convenient, but ReaderWriterLockSlim will provide better performance.

I would also suggest that you re-code the above to be:

Bitmap bmp;
if (Cache.TryGetValue(fileName, out bmp))
{
    onGetImage(fileName);
}

If you're running .NET 4.0, then I would strongly suggest that you look into using ConcurrentDictionary.

Answer 2

Why don't you just keep a a collection of image filenames that are being downloaded, and have the code for a thread be:

public void GetImage(string fileName, Action<Bitmap> onGetImage) 
{ 
    lock(Cache) 
    { 
        if (Cache.ContainsKey(fileName)) 
        { 
            onGetImage(Cache[fileName]); 
        } 
        else if (downloadingCollection.contains(fileName))
        {
            while (!Cache.ContainsKey(fileName))
            {
                System.Threading.Monitor.Wait(Cache)
            }
            onGetImage(Cache[fileName]); 
        }
        else 
        { 
           var server = new Server(); 
           downloadCollection.Add(filename);
           server.ImageDownloaded += (s, e) => 
           { 
              lock (Cache)
              {
                  downloadCollection.Remove(filename);
                  Cache.Add(e.Bitmap, e.Name); 
                  System.Threading.Monitor.PulseAll(Cache);
              }
              onGetImage(e.Bitmap);

           } 
           server.DownloadImageAsync(fileName, onGetImage); // last arg is just passed to the handler 
        } 
    } 
}

That is more or less the standard monitor pattern, or would be if you refactored the lambda expression into a member function like GetImage. You should really do that. It will make the monitor logic easier to reason about.

Answer 3

The lambda expression is converted into a delegate within a lock, but the body of the lambda expression will not automatically acquire the lock for the Cache monitor when the delegate is executed. So you may want:

server.ImageDownloaded += (s, e) =>
{
    lock (Cache)
    {
        Cache.Add(e.Bitmap, e.Name);
    }
    onGetImage(e.Bitmap);
}