Send / receive wrapper for System.Net.Sockets - follow-up

Question

I recently posted a question about improving thread and socket safety in my NetworkEndpoint class. previous post

I have implemented the changes suggested in the answer I got there. When I asked for a second look to confirm my changes I was told to post another question.

Included below is the code I changed as well as a few follow up questions. To see the entire class please refer to my previous post.

1. Is my use of locks in the Connected property and Send and Disconnect methods effective?

private object connectedLock = new object();
public bool Connected {
    get {
        lock (connectedLock) {
           if (connection == null) return false;
           return connection.Connected;
        }
    }
}

private object sendLock = new object();
public void Send(NetworkHeader header, byte[] data) {
    if (!Connected) throw new InvalidOperationException("NetworkEndpoint must be connected before sending.");
    try {
        lock (sendLock) {
            connection.Send(ByteUtils.Combine(header.Serialize(), data));
        }

    } catch (SocketException) {
        Disconnect();
    }
}

private object disconnectLock = new object();
public void Disconnect() {
   if (Connected) {
        lock (disconnectLock) {
            connection?.Shutdown(SocketShutdown.Both);
            connection?.Close();
            connection = null;
            OnDisconnected();
            Clear();
       }
    }
}

2. Is the lock I added to InitializeReceiveLoop redundant due to if (Receiving) return;?

private object initializeLock = new object();
public void InitializeReceiveLoop() {
    lock (initializeLock) {
        if (Receiving) return;
        Receiving = true;
    }
    BeginReceive();
}

3. Is this the best way to clear events? Would setting the events to null achieve the same goal?

public void Clear() {
    foreach (Delegate d in DataReceived.GetInvocationList())
        DataReceived -= (EventHandler<NetworkReceiveEventArgs>)d;
    foreach (Delegate d in Disconnected.GetInvocationList())
        Disconnected -= (EventHandler)d;
}

4. Is this implementation of EndReceive better than my previous?
5. Is EndReceive not already "thread-safe" by nature of how it is used? It is a private method only ever called by BeginReceive which is only called after the completion of a previous call to EndReceive or by InitializeReceiveLoop which can only be called once (returns imediately if Receiving is already true)

private void EndReceive(IAsyncResult result) {
    byte[] dataBuffer = null;
    NetworkHeader header = null;

    try {
        if (connection.EndReceive(result) > 0) {
            header = NetworkHeader.Deserialize(headBuffer);
            dataBuffer = new byte[header.DataSize];

            int offset = 0;
            while (offset < header.DataSize) {
                int lengthRead = connection.Receive(dataBuffer, offset,
                    (int)header.DataSize - offset, SocketFlags.None);

                if (lengthRead == 0) {
                    Disconnect();
                    return;
                }

                else offset += lengthRead;
            }
        }
    } catch (SocketException) {
        Disconnect();
        return;
    }

    OnDataReceived(header, dataBuffer);
    BeginReceive();
}

Answer 1

Events can be cleared much simpler:

public void Clear() {
    foreach (Delegate d in DataReceived.GetInvocationList())
        DataReceived -= (EventHandler<NetworkReceiveEventArgs>)d;
    foreach (Delegate d in Disconnected.GetInvocationList())
        Disconnected -= (EventHandler)d;
}

public void Clear() {
    DataReceived = null;
    Disconnected = null;
}

---

This is by no means a full review of your code, but I would like to explain a couple of things to you.

You are using seperate locks for sending, checking connection status, initializing and disconnecting. Think about the implications by using a seperate lock for operations that all require a common thread-aware resource Connected. Since they all take a different lock, they can simultaniously change the state and corrupt the other ongoing operation. This is why you should use a single lock for all these operations.

private object syncRoot = new object();

But even then, since you check the condition before the lock, data integrity might still be breached by an unfortunate flow of events.

public void Send(NetworkHeader header, byte[] data) {
    if (!Connected) // ..      // <- outside lock
    lock (syncRoot) {          // <- inside lock, but condition might no longer be true
        // ..
    }
}

As an example, suppose two threads simultaniously call respectively Send and Disconnect. A possible flow is:

- thread A: call Send                    // thread A is first
- thread A: check IsConnected: true
- thread B: call Disconnect              // thread B starts a fraction later
- thread B: check IsConnected: true
- thread B: take lock
- thread B: set IsConnected: false
- thread B: release lock
- thread A: take lock                    // thread A reaches a point where it 
             // expected its state to still be valid, but thread B changed it
- thread A: connection.Send(..)          // <- null-reference exception

There used to be a recommedation about double-checked locking. But here is a good reason not to use it. So we will have to take the condition inside the lock.

public void Send(NetworkHeader header, byte[] data) {
    lock (syncRoot) { 
        if (!Connected) // ..   
        // ..
    }
}

Moving on, we can change Connected to take a volatile boolean, so we don't require additional locking when just getting its value. Just make sure to set its value when connection is established and reset on Disconnect. The volatile field is read as an atomic operation. You get the actual value, rather than a cached value (which might be done to optimize state in a non-concurrent context).

private volatile bool _connected = false;

public bool Connected => _connected;   // <- no locking required

I hope this explains some basic concepts of locks and thread-safety.