Socket Locking across Threads

Question

Is it a good idea in my ReceiveCallBack of an asynchronous socket to Lock() the socket there? I'm asking because it's possible that another thread is sending data on the socket at the same time.

private void ReceiveCallback(IAsyncResult ar)
{
StateObject state = (StateObject)ar.AsyncState;
Socket client = state.workSocket;

lock(client)
{
    int bytesRead = client.EndReceive(ar);
    // do some work

    // Kick off socket to receive async again.
    client.BeginReceive(state.buffer, 0, StateObject.BufferSize, 0,
            new AsyncCallback(ReceiveCallback), state);
}
}    

// This is commonly called by another thread
public void SendMessage(string cmdName, Object data)
{
    lock (client)
    {
         client.Send(arrayofdata, 0, arraylength, 0);
    }
}

Answer 1

If you want to make it thread-safe and be able to send and receive simultaneously you need to create two lock sync objects:

private readonly object sendSyncRoot = new object();
private readonly object receiveSyncRoot = new object();

private void ReceiveCallback(IAsyncResult ar)
{
    StateObject state = (StateObject)ar.AsyncState;
    Socket client = state.workSocket;

    lock (receiveSyncRoot)
    {
        int bytesRead = client.EndReceive(ar);
        // do some work

        // Kick off socket to receive async again.
        client.BeginReceive(state.buffer, 0, StateObject.BufferSize, 0,
                new AsyncCallback(ReceiveCallback), state);
    }
}

// This is commonly called by another thread
public void SendMessage(string cmdName, Object data)
{
    lock (sendSyncRoot)
        client.Send(arrayofdata, 0, arraylength, 0);
}

It is generally a good practice to have dedicated syncRoot objects instead of locking on other classes or members. This allows to avoid subtle deadlocks.

Answer 2

No. Do not do that. The best way to handle a socket is encapsulation. Do not expose it to anything but the class declaring it. By doing so it's easy to make sure that only one receive is pending at a time. No need to use a lock for it.

As for the sending. do something like this:

public class MyClient
{
    private readonly Queue<byte[]> _sendBuffers = new Queue<byte[]>();
    private bool _sending;
    private Socket _socket;

    public void Send(string cmdName, object data)
    {
        lock (_sendBuffers)
        {
            _sendBuffers.Enqueue(serializedCommand);
            if (_sending) 
                return;

            _sending = true;
            ThreadPool.QueueUserWorkItem(SendFirstBuffer);
        }
    }

    private void SendFirstBuffer(object state)
    {
        while (true)
        {
            byte[] buffer;
            lock (_sendBuffers)
            {
                if (_sendBuffers.Count == 0)
                {
                    _sending = false;
                    return;
                }

                buffer = _sendBuffers.Dequeue();
            }

            _socket.Send(buffer);
        }
    }
}

This approach do not block any of the callers and all send requests are processed in turn.