1

Most code examples I've seen of locking use a pattern like this:

private static int _counter = 0;

private static readonly object _sync = new object();

public void DoWork()
{
    int counter;
    lock (_sync)
    {
        counter = _counter++;
    }
    // etc ...
}

My guess is that Montor.Enter uses some sort of reference pointer to the object that lives in memory to build some internal dictionary of what is locked by what thread. Not sure if this is correct, however.

I'm wondering if there are any ramifications of using a more complex object in the Monitor.Enter parameter. For example, if multiple threads were trying to broadcast to a WebSocket, it would be necessary to either

  1. Queue up the requests and have a single thread be responsible for sending, or
  2. Use locking to prevent multiple threads from sending to the same socket.

Suppose the WebSocket object itself was used for the lock:

public async Task SendMessage(WebSocket socket, ArraySegment<byte> data)
{
    lock (socket)
    {
        if (socket.State == WebSocketState.Open)
        {
            await socket.SendAsync(
                data,
                WebSocketMessageType.Text,
                true,
                CancellationToken.None);
        }
    }
}

If Monitor.Enter simply uses a reference pointer to the underlying object in memory, there would theoretically be no side effects to the fact that it is a big, complex object, instead of a tiny little new object().

Does anyone have any data on this?

Edit: After some answers below, I've come up with an alternative pattern, extending the WebSocket example. Any further feedback would be appreciated.

  1. A thin wrapper around the underlying object allows for the creation of a private readonly object to use for locking.
  2. The async method inside the lock is made synchronous.

Note that this pattern doesn't take into account the suggestion of only allowing a single thread to have access to the WebSocket connection (through a queue system) -- I'm mostly trying to work through my understanding of a locking pattern with a specific example.

public class SocketWrapper
{
    private readonly object _sync = new object();

    public WebSocket Socket { get; private set; }

    public SocketWrapper(WebSocket socket)
    {
        this.Socket = socket;
    }

    public async Task SendMessage(ArraySegment<byte> data)
    {
        await Task.Yield();

        lock (this._sync)
        {
            var t = await this.Socket.SendAsync(
                data,
                WebSocketMessageType.Text,
                true,
                CancellationToken.None);
            t.Wait();
        }
    }
}
Jake McGraw
  • 135
  • 4
  • 11
  • This post (http://stackoverflow.com/questions/251391/why-is-lockthis-bad?rq=1) has lots of good information regarding using other objects for locking. – itsme86 Jul 21 '16 at 23:15
  • You'd be better off using **Interlocked.Increment()** with no monitor at all in the first example –  Jul 21 '16 at 23:36

3 Answers3

3

The lock mechanism uses the header of the object to lock on, it doesn't matter how complex the object is because the header is what the mechanism is using. However a good rule of thumb for locks.

  1. Most of the time should only be locking on read-only references
  2. Create a new private object for your locks for clarity and because someone might be locking on themselves see this answer for more information
  3. Don't make your locks static unless your Method is locking at a program level

You can read more about the lock keyword and Monitor.Enter on MSDN:

Community
  • 1
  • 1
konkked
  • 3,161
  • 14
  • 19
  • Thanks very much for the info, that helps. In dealing with someone else's class (e.g. the WebSocket), it will not be possible to add a readonly property to it. Would it be correct to create a thin wrapper class that contains the original object as a property? – Jake McGraw Jul 22 '16 at 00:30
2

This is fine. .NET uses a bit of the Object header to effectively create and use a spinlock, or if that fails, it uses a pool of Semaphores.

In either case, it's based on the underlying Object header that all objects in .NET have. It doesn't matter how complex or simple the containing object is.

Anon Coward
  • 9,784
  • 3
  • 26
  • 37
1

My guess is that Montor.Enter uses some sort of reference pointer to the object that lives in memory to build some internal dictionary of what is locked by what thread. Not sure if this is correct, however.

As others have noted, there's actually a Monitor built-into every single .NET reference type. There's not an actual "dictionary" (or any other collection) of what is held by any thread.

I'm wondering if there are any ramifications of using a more complex object in the Monitor.Enter parameter.

Using any reference type is fine. However...

multiple threads were trying to broadcast to a WebSocket

In this kind of situation, queueing is preferred. In particular, await cannot exist inside a lock. It's possible to do a kind of implicit queueing by using an async-compatible lock, but that's a whole other story.

Also, it's not recommended to lock on an argument. If this example was synchronous, it would still be not recommended:

// NOT recommended
public void SendMessage(WebSocket socket, ArraySegment<byte> data)
{
  lock (socket)
  ...
}

There are some lock guidelines that have developed over the years:

  • Locks should be private. Since any code can take a lock, as soon as you lock an instance that is accessible by any other code, you open up the possibility of a deadlock. Note that it is the privacy that is important in this rule, so lock(this) is generally understood to be "not recommended", but the reason is not because you "shouldn't lock this", but rather because "this is not private, and you should only lock private instances".
  • Never call arbitrary code while holding a lock. This includes raising events or invoking callbacks. Again, this opens up the possibility of a deadlock.
  • The code within a lock (in a "critical section") should be as short as possible.
  • It's generally best to have an explicit "mutex" object (i.e., _sync), for code readability and maintainability.
  • The "mutex" should be documented as to what other object(s) it is protecting.
  • Avoid code that needs to take multiple locks. If this is unavoidable, establish and document a lock hierarchy so that locks are always acquired in the same order.

These rules naturally result in the common mutex code:

private readonly object _sync = new object();
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • Stephen, thanks so much for the insight. I've modified the original post with another code pattern, if you wouldn't mind taking a look. – Jake McGraw Jul 22 '16 at 00:45
  • @JakeMcGraw: Assuming the `async` and `await` are all removed (leaving just a synchronous method), that looks fine. – Stephen Cleary Jul 22 '16 at 01:09
  • Unfortunately for my example above, the .NET WebSocket class does not have a synchronous Send method. In a case such as this, is there a better alternative than using Task.Wait() to force it into being synchronous? – Jake McGraw Jul 22 '16 at 01:14
  • @JakeMcGraw: There's no good, universal technique to call asynchronous code synchronously. I was referring rather to the extra `async`/`await`, `Task.Yield`, and all that stuff. – Stephen Cleary Jul 22 '16 at 01:35