What would cause this property to occasionally throw a NullReferenceException?

Question

I have an asp.net/C# class that resizes images for caching on the server as files, however the portion of the code that determines which encoder to use seems to occasionally throw a NullReferenceException.

Here is the code which initializes and passes back the encoders:

public static class ImageUtilities{    
    private static Dictionary<string, ImageCodecInfo> encoders = null;

    public static Dictionary<string, ImageCodecInfo> Encoders{
        get{
            if (encoders == null){
                encoders = new Dictionary<string, ImageCodecInfo>();
            }

            //if there are no codecs, try loading them
            if (encoders.Count == 0){
                foreach (ImageCodecInfo codec in ImageCodecInfo.GetImageEncoders()){
                    encoders.Add(codec.MimeType.ToLower(), codec);
                }
            }

            return encoders;
        }
    }
    ...

This is the specific line the exception is being thrown on:

encoders.Add(codec.MimeType.ToLower(), codec);

This is the error text:

Object reference not set to an instance of an object.
    at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
    at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)

This is the only place where the Encoders property is called (and subsequently is the line below that one in the stack trace):

if (Encoders.ContainsKey(lookupKey)){
    foundCodec = Encoders[lookupKey];
}

Even if lookupKey were null, shouldn't the lookup just return null rather than throwing an exception?

Mandi

There is a good chance that this code is called from multiple threads (since post includes ASP.NET tag) and hence should be failing that way due to lack of proper synchronization. – Forint

Answer 1

You're attempting to use a "lazy loaded singleton", but you're not taking concurrency into account. The easiest way to do this without sacrificing performance is with Lazy<T>:

private static Lazy<Dictionary<string, ImageCodecInfo>> _encoders =
    new Lazy<Dictionary<string, ImageCodecInfo>>(() =>
        ImageCodecInfo.GetImageEncoders().ToDictionary(x => x.MimeType.ToLower(), x => x));

public static Dictionary<string, ImageCodecInfo> Encoders
{
    get { return _encoders.Value; }
}

This is pattern #6 of Jon Skeet's excellent article on the various ways you can implement this pattern.

You might also consider using a read-only dictionary, to prevent any callers from trying to add to it.

private static Lazy<ReadOnlyDictionary<string, ImageCodecInfo>> _encoders =
    new Lazy<ReadOnlyDictionary<string, ImageCodecInfo>>(() =>
        new ReadOnlyDictionary<string, ImageCodecInfo>(
            ImageCodecInfo.GetImageEncoders()
                .ToDictionary(x => x.MimeType.ToLower(), x => x)));

public static IReadOnlyDictionary<string, ImageCodecInfo> Encoders
{
    get { return _encoders.Value; }
}

Another way you might handle this is with a ConcurrentDictionary, but that seems like overkill since you're not going to be adding items frequently.

Jimenez

The framework will do it for you with ConcurrentDictionary – Sherilyn

@Sherilyn - yes, but that's overkill for this scenario. (Updated my answer right as you were commenting.) – Jimenez

After looking at the question further you're right--your edit exposing the dictionary as readonly is the best.You can however refactor it--just returning _encoders.Value should work – Sherilyn

Yes, but then one can cast it back to a Dictionary and add to it. Encapsulating in a ReadOnlyDictionary is a good practice to prevent that. Though I suppose in this case, it could be done once, during the lazy-load, instead of in the property. (Either would be acceptable, but updated to use this approach for efficiency.) – Jimenez

Answer 2

Since this code is in ASP.NET app, there may be some issues with concurrency. Try put creating dictionary int lock statement:

private static object _lock = new object();
public static Dictionary<string, ImageCodecInfo> Encoders{
    get{
       lock(_lock) {
        if (encoders == null){
            encoders = new Dictionary<string, ImageCodecInfo>();
        }

        //if there are no codecs, try loading them
        if (encoders.Count == 0){
            foreach (ImageCodecInfo codec in ImageCodecInfo.GetImageEncoders()){
                encoders.Add(codec.MimeType.ToLower(), codec);
            }
        }

        return encoders;
         }
    }
}

Generally Dictionary cannot have null keys (because the call GetHashCode() on every object you put in). But because you call .ToLower() on MimeType - it is rather != null (otherwise exception would be throw much earlier). If lock doesn't solve the issue you may want to check, what value you actually put into dictionary using debugger.

Astridastride

Answer 3

This can be simplified since the encoders are not going to change each time you call. Here is a version that will return the encoders as a dictionary and cache them in your local dictionary object

public static Dictionary<string, ImageCodecInfo> Encoders
{
    get {
        return encoders ??
               (encoders = ImageCodecInfo.GetImageEncoders().ToDictionary(c => c.MimeType.ToLower()));
    }
}

Lambda