CODEWITHSUNDEEP
asp.netcaching
Lieven Cardoen
Lieven Cardoen

Reputation: 25959

Review of a locked part of an function in C#.NET

Is this piece of code where I lock a part of the function correct? Or can it have use drawbacks when multiple sessions ask concurrently for the same Exam?

Purpose is that client that first asks for the Exam will assemble it, all next clients will get the cached version.

public Exam GetExamByExamDto(ExamDTO examDto, int languageId)
{
    Log.Warn("GetExamByExamDto");
    lock (LockString)
    {
        if (!ContainsExam(examDto.id, languageId))
        {
            Log.Warn("Assembling ExamDto");
            var examAssembler = new ExamAssembler();
            var exam = examAssembler.createExam(examDto);

            if (AddToCache(exam))
            {
                _examDictionary.Add(examDto.id + "_" + languageId, exam);
            }
            Log.Warn("Returning non cached ExamDto");
            return exam;
        }
    }
    Log.Warn("Returning cached ExamDto");
    return _examDictionary[examDto.id + "_" + languageId];
}

I have a feeling that this isn't the way to do it.

Upvotes: 0

Views: 97

Answers (4)

Andrew Bezzub
Andrew Bezzub

Reputation: 16032

Also you can use double check (after the exam is cached Monitor.Lock won't be invoked at all):

public Exam GetExamByExamDto(ExamDTO examDto, int languageId)
{
    Log.Warn("GetExamByExamDto");
    if (!ContainsExam(examDto.id, languageId))
    {
        lock (LockString) // Here should be some private locking object.
        {
            if (!ContainsExam(examDto.id, languageId))
            {
                Log.Warn("Assembling ExamDto");
                var examAssembler = new ExamAssembler();
                var exam = examAssembler.createExam(examDto);

                if (AddToCache(exam))
                {
                    _examDictionary.Add(examDto.id + "_" + languageId, exam);
                }
                Log.Warn("Returning non cached ExamDto");
                return exam;
            }
        }
        Log.Warn("Returning cached ExamDto");
        return _examDictionary[examDto.id + "_" + languageId];
    }
}

Upvotes: 1

Henk Holterman
Henk Holterman

Reputation: 273264

It looks basically OK but you should not lock on a string. Interning makes it hard to control which instance is which. Just create a separate object to lock on:

 private object padLock = new object();

Upvotes: 2

ilivewithian
ilivewithian

Reputation: 19702

The LockString variable, is it really a string? You shouldn't lock on a string, as you may end up with problems to do with string interning that will cause multiple locks to lock up.

The other thing is that there seems to be a lot of code inside the lock, meaning that you lock for longer than you may have to. If you can, try to get as much code out of the lock as possible.

Upvotes: 2

Oded
Oded

Reputation: 499012

Never lock on strings - they are immutable and interned, so when trying to access the same string elsewhere you may end up locking your whole application.

Just use a new object as your lock:

private readonly object Padlock = new object();

See this blog post by Tess Ferrandez.

Upvotes: 5

Related Questions