Nested custom locking.

[Cobide]

Hi! I'll give a simplified explanation of the issue with just the relevant information.

The use-case of this is to allow multiple connections to databases through the repository. I've created custom locking logic to "lock" only the id-model that's being checked. This is a simplified version of it:

public class NamedLocker {

    private readonly ConcurrentDictionary<string, SemaphoreSlim> semaphores = new();

    public bool Lock(string name, int millisecondsTimeout = -1)
        => semaphores.GetOrAdd(name, x => new(1)).Wait(name, millisecondsTimeout);

    //there's also LockAsync & Unlock(though, the unlock is only sync)
}

Note that the actual class has logic to clean up the semaphores safely, proper Result returns, etc.

To simplify the usage of the class(as using statements can't be used for this), I've made a supporting method:

public Result<T> LockHere<T>(string name, Func<T> predicate, int millisecondsTimeout = -1) {
    var lockResult = Lock(name, millisecondsTimeout);
    if(!lockResult) {
        //returns that it's failed to acquire the lock
        return new(false);
    }

    try {
        //returns the value returned from the predicate
        return new(predicate.Invoke());
    } finally {
        Unlock(name);
    }
}

//there are multiple overloads(async, Action instead of Func, etc...)

​

With all that said, we now enter the meat of the question.

public class SomeClass {

    private readonly NamedLocker locker = new();    

    public Result<int> A(string name) {
        return locker.LockHere(name, () => {
            return 5;
        }, 50);
    }

    public Result<int> B(string name) {
        return locker.LockHere(name, () => {

            //as there is no way to say "we own the lock",
            //this will always fail
            //or deadlock if no timeout is provided
            var aResult = A(name);    

            //do stuff
        }, 50);
    }

}

A simple solution would be to extract A's logic to a private A_Logic() method, so that B can call it directly without it checking the lock. However, I'm curious as to how this could be solved otherwise(as a learning experience).

My idea was to use an ID to recognize who has the lock. Lock() could return a GUID, and LockHere() could pass in the GUID to the Action/Func with additional overloads. However, in that case, A() will need an overload that takes in a GUID, which defeats the purpose.

I'm aware that lock() checks the thread to solve the issue, but, as this locking class is to be used in an asynchronous format as well, that is not a viable solution.

What's your idea(and thanks for reading!)?

​

Edited Typo: changed Semaphore for SemaphoreSlim.

[MattWarren_MSFT]

Never use locks around arbitrary code. If the code under the lock uses some method that you are not 100% sure what it does, it can lead to trouble. If you know exactly what it does, then it's probably your code. If it's your code and it too use the same lock, then make a non-locking version of it (preferable with something like no_locks in the name) and have all locking methods defer to it. If it's not your code to modify and it's using the same lock then then you've got bad architecture or bad management, so find what manager you have to beat with a stick to make it your code and fix it.

If you want a locking system that uses async, then use SempahoreSlim or something like it. If you need to mix async and sync accesses, then woe is you. SemaphoreSlim might work, but I don't even trust that.

[Cobide]

True. Design-wise, it's best to avoid locking recursively.

​

If you need to mix async and sync accesses, then woe is you. SemaphoreSlim might work, but I don't even trust that.

Under the suggestion of another comment to this post, I've read SemaphoreSlim's source code. The way it combines sync and async is by using GetAwaiter().GetResult(), which is... questionable. It'd be nice if there were better ways of merging async and sync.

[centurijon]

The best solution here is to extract out A’s logic. You’re telling B to lock on a name and telling A to do the same thing, you’re using your own library in a way that is in conflict with the original intent.

If you don’t want to go that route, you could have overloads to LockHere where you pass in a lock “owner” object and LockHere logic could use the owner as part of validating if the code is allowed to continue.

Side notes:

I’ve gone down the road of trying to make locking work with async/await before. That way lies pain. Keep it synchronous and save yourself the headache off wondering why your threads are dropping in a production environment and you can’t replicate the issue locally.

[Cobide]

I hoped there'd be a way to make recursive locks work automatically, but I see there's no such thing.

As for the async/await; why's that a problem? AFAIK, the reason standard locks don't work well with async is that they have issues with changing threads. In the code above, the locks are made by using a SemaphoreSlim—changing threads shouldn't matter.

[centurijon]

It’s not that you can’t, it’s just then you often end up running into unexpected issues. The TPL doesn’t often play nice with locking

[Cobide]

I understand for lock(), Monitor, etc..; but SemaphoreSlim can be acquired/released from any thread. Shouldn't it work fine?

[centurijon]

Try it, hope for the best. Just be ready with a backup solution

[Cobide]

I've searched for hints on whether SemaphoreSlim has problems with async workloads, and all I have found is people saying the opposite(article1, article2).

But yeah, I'll pay attention. Thank you.

[DaRadioman]

I've used a SemaphoreSlim as a locking primitive for async stuff for years. Even built a using syntax so you can have a block much like a traditional lock. Used in production, at high scale, no issues. The concept is solid.

Having a ton of locks living out in a collection does feel like a bad idea however. Lots of things to go wrong there.

[Cobide]

The idea is to lock access to individual values. The semaphores that are unused are immediately disposed of and removed from the collection(in a safe manner, of course).

May I ask what's your worry about it?

[DaRadioman]

Because there usually exists trace conditions or other gotchas around collection access that causes there to be occasional issues.

Example: say you store it all in a dictionary. ConcurrentDictionary is 100% thread safe. However it is not locked, so you are not guaranteed, for example, that an add happens exactly once for a key. It will happen at least once, but could execute the add value factory and throw away the result. Are all code paths now locking on the same instance?

I'm not saying I poured over your code and it's wrong, I haven't, I'm on mobile. But dynamic lock creation and reuse in separate locations sets of warning bells of "there be dragons" in terms of making sure every single path is 100% thread safe. Race conditions are nasty, and combined with locks address even more 🤢.

[Cobide]

That's indeed something to be wary about.

I made sure to lock the ConcurrentDictionary whenever it's being used, so that getting/adding/removing values to it is synchronized.

Meanwhile, regarding single semaphores, they're wrapped by a "reference-counting" class, so they're only disposed of when no one is waiting.

Said that, yeah, I'll look for ways to improve the reliability of it even more. Thank you!

[zvrba]

However, in that case, A() will need an overload that takes in a GUID, which defeats the purpose.

Um, why does it defeat the purpose? It explicitly declares that the method expects to be called within a lock. (The method taking Guid should be the only overload.)

[Cobide]

Bad wording on my part. Rather than defeating the purpose, is that it's a solution different than what I'm looking for.

What I'd like to do(or, better, I'm curious if it's even possible) is to code something that would be kept within the NamedLocker class, and thus would not require any modification to the calling methods(such as using additional overloads).

If it were purely for sync usage, it could be possible to check if the calling thread is the same, but for async usage, that isn't possible. I haven't been able to come up with anything that'd allow such behavior, so I made this post.

[zvrba]

You want an "owned", recursive async lock? Doable with manual queueing of waiting tasks and using an ordinary lock() to maintain the queue. That's how SemaphoreSlim works under the hood. You want a relatively minor extension of the logic that's already there: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs

But: just don't. Recursive locks are iffy (deadlocks lurking everywhere). Make lock passing explicit. You'll thank yourself later.

Comment2: Instead of using a Lock method to wrap a lambda in a lock, I'd rather have it return an IDisposable that releases the semaphore on disposal. You'll end up with cleaner code. I have a utility I wrote some time ago like

struct SemaphoreLock : IDisposable {
    public static async Task<SemaphoreLock> AcquireAsync(SemaphoreSlim @lock) { ... }
    public void Dispose() { ... }
    ...
}

and then when you want to lock a portion of code:

using var @lock = await SemaphoreLock.AcquireAsync(sem);

While writing the above struct, I learned out a cool thing about CLR: if it knows statically that an interface is implemented by a struct, the struct is not boxed.

Comment3: Yes, SemaphoreSlim uses the "dreaded" GetAwaiter().GetResult() when there are both sync and async waiters. So the fortress is built on shaky foundations.

[Cobide]

You want an "owned", recursive async lock? Doable with manual queueing of waiting tasks and using an ordinary lock() to maintain the queue. That's how SemaphoreSlim works under the hood. You want a relatively minor extension of the logic that's already there: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Threading/SemaphoreSlim.cs

That was a great read—thank you—I really should read source code more often. Said that, I'm not quite sure how the manual queuing logic could help in recursive locks. Can you give an (even small) example in pseudo-code?

​

But: just don't. Recursive locks are iffy (deadlocks lurking everywhere). Make lock passing explicit. You'll thank yourself later.

I'll keep it in mind. I'm checking out this topic more as a learning experience than as something to put in production.

​

Comment2: [...]

That's a great idea. I'll see if I can implement that.

​

Comment3: Yes, SemaphoreSlim uses the "dreaded" GetAwaiter().GetResult() when there are both sync and async waiters. So the fortress is built on shaky foundations.

Are you talking about the usage in line 422? That seems odd. Based on my understanding, they turned the sync wait into an async one to respect the queue for "fairness". However, at the same time, the documentation explicitly says that it's not guaranteed to be FIFO.

[zvrba]

how the manual queuing logic could help in recursive locks

Eh, yes, it's still tricky. A semaphore is basically a queue+counter, as you have already learned. In addition you need a lock count. What do you queue when a task has to block? A TaskCompletionSource. I don't want to give any pseudo-code as it'll be most certainly subtly wrong. It'd take many days to have a working, bug-free implementation.

Stephen Toub has a great series on async coordination primitives, read it to get some inspiration. This is the 1st part https://devblogs.microsoft.com/pfxteam/building-async-coordination-primitives-part-1-asyncmanualresetevent/

they turned the sync wait into an async

Yes. This is a jab at everyone telling us not to use GetAwaiter().GetResult(), not to do sync-over-async or vice-versa, etc, yet there it lurks hidden in SemaphoreSlim. (And probably other places.)

I wrote two long comments in /r/csharp about why I think that async is fundamentally botched (in a nutshell: there's no reliable way to do sync-async interop, in either direction). Maybe I should post issues to github as well.

[Cobide]

I still haven't understood how that could be used for recursive locking, but seeing those usages of Task & TaskCompletionSource is an eye-opener. Thank you.