SqlServerDistributedLock won't work correctly in async task

[pieceofsummer]

It is currently not an issue for DisableConcurrentExecutionAttribute, since it wraps a parent task (which Hangfire runs synchronously), but it can become one in the future.

Consider the following test:

private async Task AsyncChildTask()
{
    using (var connection = ConnectionUtils.CreateConnection())
    {
        var storage = CreateStorage(connection);
        using (var @lock = new SqlServerDistributedLock(storage, "test", _timeout))
        {
            var AcquiredLocksField = typeof(SqlServerDistributedLock)
                .GetField("AcquiredLocks", BindingFlags.Static | BindingFlags.NonPublic);

            var AcquiredLocks = (ThreadLocal<Dictionary<string, int>>)AcquiredLocksField.GetValue(null);

            Assert.True(AcquiredLocks.Value.ContainsKey("test"));

            await Task.Yield();

            Assert.True(AcquiredLocks.Value.ContainsKey("test"), "Lock is not owned"); // False!
        }
    });

}

[Fact]
public void ParentTaskCallingAsyncChild()
{
    Task.WaitAll(AsyncChildTask());
}

It will fail the second assertion, because task may continue on different thread, so using ThreadLocal<> is incorrect for storing owned locks. You should use AsyncLocal<> instead (CallContext on older frameworks).

[odinserj]

Nice catch, @pieceofsummer! That's also a case for the JobActivatorScope class. Unfortunately, AsyncLocal<> type is available only starting from .NET 4.6. How can we implement this in 4.5? And what's better, to have own implementation of AsyncLocal, or to simply bump the required framework version in Hangfire 2.0.0?

[pieceofsummer]

Not sure which is better (but keeping 4.5 support is good, since it can run on Mono too), but many projects (e.g MongoDB driver) use a local implementation like this:

namespace Xxx
{
#if NET45

    using System.Runtime.Remoting.Messaging;

    internal class AsyncLocal<T>
    {
        private readonly string __name;

        public AsyncLocal()
        {
            __name = Guid.NewGuid().ToString();
        }

        public T Value
        {
            get
            {
                var value = CallContext.LogicalGetData(__name);
                return value == null ? default(T) : (T)value;
            }
            set
            {
                CallContext.LogicalSetData(__name, value);
            }
        }
    }

#endif
}

[odinserj]

How simple, but still unsure whether it is safe or not ☺️ Of course, the fact that others are using it makes sense, but we should learn the caveats first – CallContext is one of the deepest classes in threading world in .NET, and we should use it with care. Let's schedule this to 2.0, time will tell whether it's good or bad.

[pieceofsummer]

Well, after some research I can say they look pretty much the same.

Both LogicalCallContext and local values are stored in the ExecutionContext currently associated with the thread. The only difference is the level of indirection (logical data values are stored in the hashtable inside LogicalCallContext, while async locals are stored in dictionary right in ExecutionContext).

[odinserj]

@pieceofsummer, I've looked into the source code and realized that it's better to make the lock reentrancy feature using the SqlServerConnection class itself, rather than using the ThreadLocal, AsyncLocal, etc. mechanisms in the distributed lock class. The current logic is crappy and prone to errors (dictionary instances aren't released at all currently).

For example, we could add an instance-scoped HashSet object to the SqlServerConnection class, and return NoOpDistribtuedLock with no logic at all, if a lock for a resource was already obtained. The resulting logic will be much easier to understand.

However, there is another usage of the ThreadLocal class that would benefit from AsyncLocal, and it is in the JobActivatorScope class.

If you'd like to make these changes, I'll be happy to merge pull requests!

[pieceofsummer]

Btw, here's another proof using LogicalCallContext is correct: https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http/HttpContextAccessor.cs

I have looked at JobActivatorScope implementation, and I have some concerns about it:

• It sets current scope to null on dispose instead of restoring a previous value, which makes it not reentrant. Is this a correct behavior?
• I don't see JobActivatorScope.Current to be actually used anywhere. Is it needed at all?

For reference, .NET Core avoids static properties like Current as much as possible. Probably we should avoid them too, as they make things messy.

[odinserj]

You've convinced me, I'll be happy to accept the PR regarding AsyncLocal ☺️

It sets current scope to null on dispose instead of restoring a previous value, which makes it not reentrant. Is this a correct behavior?

Nope, looks like a future bug.

I don't see JobActivatorScope.Current to be actually used anywhere. Is it needed at all?

It's used in Hangfire.Ninject and all the other containers that doesn't provide nested scopes directly, relying on a static property.

[pieceofsummer]

One more question: what is InnerScope from JobActivatorScope used for?

If it is appears to be a misnamed outer (parent) scope, it should at least be readonly, since you cannot change parent scope from the child scope.

And if it is actually supposed for parent scope to hold a reference to an inner (child) scope, it doesn't make sense at all, since no one will actually use its value.

[odinserj]

One more question: what is InnerScope from JobActivatorScope used for?

Looks like it was added by a mistake and should be removed. But of course after ensuring that no other IoC container integration use it. Could you make a PR for this? I like your work for clarifying and reviewing things 👍 Sorry for delayed answers, I'll try to give them faster.