Send / receive wrapper for System.Net.Sockets

Question

The following NetworkEndpoint class serves as a simple send / receive interface for either side of a network connection (client or server) when provided with a socket to operate through.

I have two main concerns about the code in it's current state:

1. What can I do to improve to exception handling to better deal with the socket becoming disconnected (both unexpectedly and intentionally)?

2. As far as I can tell I have protected against multiple receive loops begin started. (if (Receiving) return; in void InitializeReceiveLoop()) but are there any other major thread safety issues I have overlooked??

My knowledge of C# is primarily self taught so I apologize if my code does't reflect best practices or standard style guides.

Any advice on either of these issues or advice in general on improving my code would be greatly appreciated.

public class NetworkEndpoint {

    private Socket connection;
    public bool Connected { get => connection != null ? connection.Connected : false; }
    public bool Receiving { get; private set; }

    public event EventHandler<NetworkReceiveEventArgs> DataReceived;
    private void OnDataReceived(NetworkHeader header, byte[] data) =>
    DataReceived?.Invoke(this, new NetworkReceiveEventArgs(header, data));

    public event EventHandler Disconnected;
    private void OnDisconnected() =>
    Disconnected?.Invoke(this, new EventArgs());

    public NetworkEndpoint(Socket socket) {
        connection = socket ?? throw new ArgumentNullException("socket");
        Receiving = false;
    }

    public void Send(NetworkHeader header, byte[] data) {
        if (!Connected) throw new InvalidOperationException("NetworkEndpoint must be connected before sending.");
        try {
            connection.Send(ByteUtils.Combine(header.Serialize(), data));
            } catch (SocketException ex) {
            Disconnect();
        }

    }

    public void Disconnect() {
        connection?.Shutdown(SocketShutdown.Both);
        connection?.Close();
        connection = null;
        OnDisconnected();
    }

    public void InitializeReceiveLoop() {
        if (Receiving) return;

        Receiving = true;
        BeginReceive();
    }

    private byte[] headBuffer = new byte[NetworkHeader.ByteSize];
    private void BeginReceive() {
        connection.BeginReceive(headBuffer, 0, NetworkHeader.ByteSize, SocketFlags.None, EndReceive, null);
    }

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

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

                int receivedLength = connection.Receive(dataBuffer, 0, dataBuffer.Length, SocketFlags.None);
                fullyReceived = (receivedLength == header.DataSize);
            }
        } catch (SocketException) { }

        if (fullyReceived) {
            OnDataReceived(header, dataBuffer);
            BeginReceive();
        } else if (connection.Connected) Disconnect();
    }
}

Answer 1

Review

• Property Connected is not thread-safe. Use Interlocked.CompareExchange to get the connection atomically.
• Events DataReceived and Disconnected are never cleared. This will cause a memory leak. Implement a Clear function and/or implement IDisposable.
• Methods Send, Disconnect, EndReceive are not thread-safe. Consider using a mutex/lock.
• Checking fullyReceived on receivedLength == header.DataSize is naive and can be broken. TCP messages can be sent in several packets. Use an internal buffer instead. Also, receivedLength = 0 is a clean way of determining no more data is available.
• Catching SocketException blindly is a code-smell.