Change scope to only acquire distributed locks once in a scope chain.

[nikolajlauridsen]

Overview

Currently, we do one SQL query every time a distributed lock is requested, even if the lock has already been acquired. This is not necessary since the lock is held until the outermost scope has been disposed and leads to a lot of unnecessary SQL queries when there is a lot of nested scopes.

This PR implements a fix where the SQL call will only be made if we haven't made a call for that specific lock object identifier yet.

Details

The way it works is that two Dictionary<int, int> has been added to the Scope class, one for read locks and one for write locks. These dictionaries serve two purposes:

1. On the outermost scope, it lists all the locks we've acquired as keys, and how many of each as the value. The amount of locks is just recorded for debugging purposes.
2. On child scopes it's used to track what, and how many, locks we've requested, this is necessary in order to be able to decrement the outermost scope's dictionary when a child scope is disposed.

When a request for a lock is made to an inner scope, it will pass that request on to its parent, and the parent will try to pass on again, and so on until there no longer is a parent to pass it to i.e. we've reached the outermost scope. The outermost scope then looks in its dictionary, if there is no key with the requested id we make the SQL query, and add a key to the dictionary, we now have the lock and know it.

That's the gist of it, I've also added new unit tests to ensure that the SQL call is actually only made once, and some to ensure that the outermost dictionary counts correctly.

Ambient scope

When we initially discussed this proposed solution we also talked about and "Auto created ambient scope" but when investigating the code I was unable to find any reference to this or an actual auto-created scope. The ambient scope of the ScopeProvider will always (as far as I can tell) be set to the most recently created child scope i.e. the innermost scope, and we do very much still want to apply this logic to this scope, otherwise, it won't have an effect.

Being somewhat uncertain I asked @Shazwazza for input, and he seemed to have come to the same conclusion, so for now, I've ignored the part about the "auto-created ambient scope"

Testing

The name of the game is to ensure that everything still works correctly, to achieve that I have:

• As mentioned added some additional unit tests.
• Run the acceptance tests when all pass except for "Create Template" and the tours ones, but those are due to well-known issues in our cypress tests and has nothing to do with this PR.
• Done some manual testing, trying to create some content with nested content and so on, all with no issues.

[Shazwazza]

I know in our meeting we decided to use only Dictionary without locks but we need to sync access to this because it is more than possible that a Scope/ScopeProvider can be used across threads. This is because with async code and Task.Run, the execution context flows and therefore so does the Scope/ScopeProvider in the current context.

This means that any access to these Dictionaries: ReadLocks/WriteLocks need to be locked. I don't think we need to go so far as using ConcurrentDictionary, we should just have an standard object lock on the class (non static) and wrap a lock(_locker) {...} statement around all access to these dictionaries. Because all access to these dictionaries is done on the outermost scope, it essentially means that only the outermost scope's lock will be used.

[nikolajlauridsen]

I've wrapped access to the dictionaries in locks, but just to clarify, the inner scopes dictionaries will also be accessed by themselves, since they're used to keep track of what locks that specific scope has requested so the dictionary on the outermost scope can be decremented once an inner scope is disposed.

When a lock is requested with the ReadLock or WriteLock methods the scope check if it's an inner scope in the IncrementRequestedWriteLock and IncrementRequestedReadLock methods, if it is it will use the dictionary to count what, and how many locks it has requested like so:

private void IncrementRequestedReadLock(params int[] lockIds)
{
    // We need to keep track of what lockIds we have requested locks for to be able to decrement them.
    if (ParentScope != null)
    {
        foreach (var lockId in lockIds)
        {
            lock (_dictionaryLocker)
            {
                if (!ReadLocks.ContainsKey(lockId))
                {
                    ReadLocks[lockId] = 0;
                }
                ReadLocks[lockId] += 1;
            }
        }
    }
}

This is then used in the Dispose method to decrement the outermost scopes dictionary:

// Decrement the lock counters on the parent if any.
if (ParentScope != null)
{
    lock (_dictionaryLocker)
    {
        foreach (var readLockPair in ReadLocks)
        {
            DecrementReadLock(readLockPair.Key, readLockPair.Value);
        }
    }

    lock (_dictionaryLocker)
    {
        foreach (var writeLockPair in WriteLocks)
        {
            DecrementWriteLock(writeLockPair.Key, writeLockPair.Value);
        }
    }
}

In the DecrementReadLock method the outermost scopes dictionary is then decremented like so:

public void DecrementReadLock(int lockId, int amountToDecrement)
{
    // If we aren't the outermost scope, pass it on to the parent.
    if (ParentScope != null)
    {
        ParentScope.DecrementReadLock(lockId, amountToDecrement);
        return;
    }

    lock (_dictionaryLocker)
    {
        ReadLocks[lockId] -= amountToDecrement;
    }
}

But I think this should still be fine, because the inner scopes own lock will be used when handling its own dictionary.

[bergmania]

Seems to work as expected 💪

[Shazwazza]

I have left some comments and questions inline and think that some of this needs some re-work. As this is critical code we need to make sure we cover all bases.

Is there a reason we are counting anything at any child scope level? Why not just increment/decrement all read/write locks only at the root?

We need to make sure that incrementing only occurs if the call to actually acquire the lock was successful else we will end up with incorrect values and bad things may happen.

[Shazwazza]

This should be ReadLocks.TryGetValue and then increment the result and assign.

[Shazwazza]

This should be WriteLocks.TryGetValue and then increment the result and assign.

[Shazwazza]

This should be an atomic operation but it isn't. If one thing fails then the value is now incorrect. Same goes with the other calls: ReadLock, WriteLock

[Shazwazza]

This has potential to be a recursive lock. There's quite a lot of recursive locking here and and I fear this will quite easily cause a deadlock. Same with DecrementWriteLock.

However, I 'think' from the looks of it that this particular recursive lock won't be recursive because the original call to DecrementReadLock (and DecrementWriteLock) come from Dispose which is lock the current scope's _dictionaryLock which isn't locked again here. It's just a little confusing.

[Shazwazza]

This comment is reversed, in this block we want an ordinary lock

[Shazwazza]

We shouldn't lock individually on each lockId that will slow things down, there should just be a single lock over this for loop. Same thing for the WriteLockInner

[Shazwazza]

I don't fully understand this check. If we are on the root Scope then nothing is decremented? Does that mean that if there are never child scopes, nothing is counted and we'll just keep making SQL lock calls for read/write locks requested only when there's an outer scope?