Modify locked object within lock block

Question

I'm having thread contention in an OLTP app. While reviewing the code involved, I found the following:

        lock (_pendingTransactions)
        {
            transaction.EndPointRequest.Request.Key = (string)MessageComparer.GenerateKey(transaction.EndPointRequest.Request);

            if (!_pendingTransactions.ContainsKey(transaction.EndPointRequest.Request.Key))
            {
                _pendingTransactions.Add(transaction.EndPointRequest.Request.Key, transaction);

                return true;
            }
            else
            {
                return false;
            }
        }

As you can see in the snippet, there is a lock on an object that is modified within the 'lock' block. Is there anything bad with that? Anyone had problems doing something like this?

Answer 1

Generally speaking, one will take a lock on an object specifically because one is going to modify (or read) it, so there's nothing inherently wrong with this.

Answer 2

Using locking in this way is often discouraged, with the recommendation being to use a dedicated lock field (class member variable). A dedicated lock field is of type Object and usually looks like this:

private object _pendingTransactionLock = new object();

If the object itself has some threading awareness, this lock variable might belong in _pendingTransaction's implementation class. Otherwise, it might belong alongside _pendingTransaction in the field's declaring class.

You don't say what type _pendingTransaction is. If this is a built-in collection class that provides a SyncRoot property, that might be a good choice to lock on.

See Jon Skeet's Choosing What to Lock On.

Answer 3

Probably the key generation can be taken outside the lock block, to reduce the duration of the lock. Other than that, this is an almost canonical example of lock that protects a list/collection/array: acquire lock, check if the key exists, add key if not already present, release the lock.