Using lock with value type (multithread context) 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&lt;TDataType, T&gt;
{
    // 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&lt;T, LockObject&gt; lockTable = new Dictionary&lt;T, LockObject&gt;();

    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&lt;TResult&gt;( T valueToLockOn, Func&lt;TResult&gt; 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&lt;AnObjectClass, Guid&gt;.Locked( anObjectId, () =&gt;
{
    // 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.
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.