Using lock with value type (multithread context)

Question

I have to use value type to lock code sections in correspondence to objects identifiers (i.e. Guid, int, long, etc.).

So I've written the following generic class/method, which uses a Dictionary to handle object that is a reference in correspondence to the value type (so this objects can be used with the lock instruction).

I think the code and comments are self-explanatory enough.

public static class Funcall<TDataType, T>
{
 // Object used for lock instruction and handling a lock counter
 class LockObject
 {
 public int Counter = 0;
 }
 // Correspondance table between the (maybe) value type and lock objects
 // The TDatatype above allows to create an independant static lockTable for each given TDatatype
 static Dictionary<T, LockObject> lockTable = new Dictionary<T, LockObject>();
 static LockObject Lock( T valueToLockOn )
 {
 lock ( lockTable ) // Globally locks the table
 {
 // Create the lock object if not allready there and increment the counter
 if ( lockTable.TryGetValue( valueToLockOn, out var lockObject ) == false )
 {
 lockObject = new LockObject();
 lockTable.Add( valueToLockOn, lockObject );
 }
 lockObject.Counter++;
 return lockObject;
 }
 }
 static void Unlock( T valueToLockOn, LockObject lockObject )
 {
 lock ( lockTable ) // Globally locks the table
 {
 // Decrement the counter and free the object if down to zero
 lockObject.Counter--;
 if ( lockObject.Counter == 0 )
 lockTable.Remove( valueToLockOn );
 }
 }
 public static void Locked( T valueToLockOn, Action action )
 {
 var lockObject = default( LockObject );
 try
 {
 // Obtain a lock object
 lockObject = Lock( valueToLockOn );
 lock ( lockObject ) // Lock on the object (and so on the corresponding value)
 {
 // Call the action
 action?.Invoke();
 }
 }
 finally
 {
 // Release the lock
 Unlock( valueToLockOn, lockObject );
 }
 }
 // Same as above except this is returning a value (Func instead of Action)
 public static TResult Locked<TResult>( T valueToLockOn, Func<TResult> action )
 {
 var lockObject = default( LockObject );
 try
 {
 lockObject = Lock( valueToLockOn );
 lock ( lockObject )
 {
 return action == null ? default( TResult ) : action();
 }
 }
 finally
 {
 Unlock( valueToLockOn, lockObject );
 }
 }
}

Which can be used like this:

Guid anObjectId = ObtainTheId();
return Funcall<AnObjectClass, Guid>.Locked( anObjectId, () =>
{
 // do something
 return something;
} );

Are there better ways to do that? Any advice is welcome.

Edit/note: as asked in the comments, the use case is the following

This "lock" system is to be used by a module for which the entry points are dedicated to modify stored objects of a particular type (this object type will be modified by this module only). Externally the module is used by several concurrent workers.

More specifically, I use MongoDB 3.4 for objects storage and it does not provides transactions which are new in version 4.0 (I mean here session.Start, session.Commit). Among that, I don't really need a full/complete transactional system, but simply ensure each step of the workers demands are each consistent at the time each demand occurs. I'm aware that this simple "lock" system can be considered as weak, but it is simple and meet my need in the context I am working on.

• user52292: Why not locking the objects directly? Why locking its GUID? What is that do something? Isn't it working with the object that you could have locked in the first place?
• lemon: @firda, typically, the object is stored in a database, so each time it is retrieved, this is a new C# instance.
• user52292: I still don't see a problem with that, unless you have multiple instances simultaneously, which sounds like a mess. Don't get me wrong, I am here to learn something as much as you are. Just curious about the design (of the usage of those locks), either single lockable instance per database object, or transactions, cannot imagine your usage.
• lemon: @firda, question edited to explain the context

Answer 1

Thread safety

I've not tested this, but it looks solid enough, since the only non-trivial logic is held in lock statements. Encapsulation is accordingly good.

Depending on the load, and if performance is critical, some reader/writer locking might help to ease contention (e.g. if the lock object already exists, you don't need to write-lock the dictionary to retrieve it, and if the count is non-zero when you release the lock, you don't need to write-lock to remove it), but it will dramatically increase the complexity.

Naming API

Lock isn't a great name: it doesn't lock anything, it just gives you a handle to a LockObject, which you happen to have to release. Unlock similarly. Personally I avoid split prepositions

Locked isn't a great method name either: Locked suggests a state, not an operation. I'd rather call these RunLocked, or RunExclusive or something.

---

I don't like this:

return action == null ? default( TResult ) : action();

It means that if action == null, then the system goes to all that effort just to return a meaningful-but-made-up value. What use case could this have? This may end up obscuring a bug later on, when something which shouldn't be null ends up being null, and this code covers up the fact by returning default(TResult). In the very least this behaviour should be clearly documented, but I'd much rather it just throw an ArgumentException with a helpful error message.

The same goes for action?.Invoke(); in the other overload.

---

As always, I'll suggest inline-documentation (///) on the type and public members: it is little (well spent) effort; makes it so much easier to exploit the API without having to leave the IDE; and improves maintainability, as anyone working on the code can see inline what it is meant to be doing and avoid breaking that contract.

---

Why is this static? I'd much sooner make this non-static, and provide a static instance if that is a meaningful concept. That way, if you need to lock on the same type for different purposes then you can. Making this static needlessly restricts the applicable use-cases.

Dodgy try...finally

The try...finally constructs in the Locked methods are a bit dodgy... Unlock will throw if lockObject is null, which means you should be entering the try knowing that it is not null. The quick-and-easy solution is to move the call to Lock out of the try. If Lock can crash (it's not immediately clear how under normal circumstances it would, thought), then you need to consider that specifically.

Other misc

• allready is misspelt in one of the comments.

• The lockTable can (and arguably should) be readonly.

• I'd consider replacing the Counter++ and Counter-- with dedicated Increment()/Decrement() methods in LockObject (making counter private), since any other usage would (presently) make no sense.

• Many people object to the if without {} construct: you only have one usage, so I'd consider putting the braces in.

• Personally I like to make the accessibility of everything explicit (i.e. mark private members and classes private); it just saves the reader having to remember anything, and avoids any confusion when coming from languages with different defaults.

• lemon: Thanks. In my opinion, the only point in your answer to be discussed a bit more is about static or not (I agree with all others). I asked myself the question about that. I use static because I did not want to suggest that the lock mecanisme was linked to any particular instance. So if a more contextual lock component was needed, I should write another code for that (say a 'ContextualLocker' or something like that). Simpler in my opinion to keep each case separated. Do you agree this approach?
• lemon: About that name of the methods, what about 'InLockSection', so that we can call it with 'Funcall.InLockSection' or 'Funcall.InExclusiveContext'?
• VisualMelon: @lemon concerning the naming, I'd just want to avoid providing a name which sounds like an adjective (e.g. InLockSection might be read as answering the question whether the key is already locked). Regarding static, it depends on your use-case, but naively I'd suggest providing a general purpose instance version (less to maintain), and then create a single well-named static instance where it is needed. Imagine you put this in a DLL: if 2 components depended on it for different jobs, they would interfere, but if they each created their own static instance, they'd get along fine.

Answer 2

This solution seems overly complex. If I'm understanding it correctly you just want to substitute a value type for a reference type to lock on.

public class LockMapper<T>
{
 private static readonly ConcurrentDictionary<T, object> _lockTable = new ConcurrentDictionary<T, object>();
 public static object GetSyncRoot(T valueToLockOn)
 {
 return _lockTable.GetOrAdd(valueToLockOn, _ => new object());
 }
}

Then in your code you can just do the following standard C# lock pattern.

lock (LockMapper<Guid>.GetSyncRoot(anObjectId))
{
 // do something
}

• lemon: If I understand well your code, this will not free the table. But the idea is interesting and maybe a disposable instance could handle that in some way. But that will reintroduce an equivalent complexity?