CODEWITHSUNDEEP
c#staticthread-safety
Pedro77
Pedro77

Reputation: 5294

Static method on a static class, is this method thread safe?

Do I need to use the lock (createPaletteLocker)? I'm using it because I think that doing so I can avoid a write to ColorPaletteHandle.palettesList[type] while other thread is reading it.

EDIT 1: The dictionary is set only at the static constructor. So it I'll not be changed (add/remove) anymore, but the stances that it holds will change from null to the respective created palette.

public static class ColorPaletteHandle
{
    private static readonly object createPaletteLocker = new object();
    private static Dictionary<ColorPaletteType, ColorPalette> palettesList = null;

    static ColorPaletteHandle()
    {
        palettesList = new Dictionary<ColorPaletteType, ColorPalette>();
        palettesList.Add(ColorPaletteType.Default, ColorPaletteHandle.defaultPalette);
        palettesList.Add(ColorPaletteType.EdgesHighlight, ColorPaletteHandle.edgesHighlight);
        palettesList.Add(ColorPaletteType.GrayScale, ColorPaletteHandle.grayScale);
        palettesList.Add(ColorPaletteType.HeatMap, ColorPaletteHandle.heatMap);
    }

    /// <summary>
    /// Gets a palette.
    /// </summary>
    /// <param name="type"></param>
    /// <returns></returns>
    public static ColorPalette GetPalette(ColorPaletteType type)
    {
        ColorPalette pal = null;
        lock (ColorPaletteHandle.createPaletteLocker)
        {
            pal = ColorPaletteHandle.palettesList[type];
            if (pal == null)
            {
                ColorPaletteHandle.palettesList[type] = ColorPaletteHandle.CreatePalette(type);
                pal = ColorPaletteHandle.palettesList[type];
            }
        }
        return pal;
    }

   //stuff...
   }

Upvotes: 2

Views: 142

Answers (2)

Jason
Jason

Reputation: 3960

Yes this method is thread safe since only one thread can modify palettesList at a time, and yes, you do need the lock if you intend on using Dictionary

But, if you are on .NET 4 or above, ConcurrentDictionary would be better suited for this

Upvotes: 2

Servy
Servy

Reputation: 203850

The Dictionary class is not designed to be accessed from multiple threads. Doing so would indeed cause problems unless you synchronized access. With the lock statement the method is fine, without it it is not.

Even better than using lock would be to use a ConcurrentDictionary instead. It is a dictionary that is specifically designed to be used from multiple threads at the same time. You'd be able to refactor your whole method to one call to GetOrAdd.

Upvotes: 6

Related Questions