8
\$\begingroup\$

I'd like to get a little feedback on this async socket wrapper. My goal is to merge socket Begin/End methods into a single async call. I also wanted to wrap exceptions so they are easier to handle in others areas of my program. Any red flags? Any potential problems? I know its not doing much but I want to get this right.

public class SimpleSocket
{
    private readonly Socket _socket;

    public SimpleSocket()
    {
        _socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    }

    public SimpleSocket(Socket socket)
    {
        _socket = socket;
    }

    public void Bind(int port)
    {
        var endPoint = new IPEndPoint(IPAddress.Any, port);

        try
        {
            _socket.Bind(endPoint);
        }
        catch (Exception e)
        {
            throw new ConnectionErrorException($"Failed to bind to {port}", e);
        }
    }

    public void Listen(int backlog)
    {
        try
        {
            _socket.Listen(backlog);
        }
        catch (Exception e)
        {
            throw new ConnectionErrorException($"Failed to listen with backlog of {backlog}", e);
        }
    }

    public async Task<SimpleSocket> AcceptAsync()
    {
        Socket socket;

        try
        {
            socket = await Task.Factory.FromAsync(_socket.BeginAccept, _socket.EndAccept, true);
        }
        catch (Exception e)
        {
            throw new ConnectionErrorException("Failed to accept connection", e);
        }

        return new SimpleSocket(socket);
    }

    public async Task ConnectAsync(string host, int port)
    {
        try
        {
            await Task.Factory.FromAsync(_socket.BeginConnect, _socket.EndConnect, host, port, null);
        }
        catch (Exception e)
        {
            throw new ConnectionErrorException($"Failed to connect to {host}:{port}", e);
        }
    }

    public async Task<int> ReceiveAsync(byte[] buffer, int offset, int size)
    {
        int bytesReceived;

        try
        {
            bytesReceived = await Task<int>.Factory.FromAsync(
                _socket.BeginReceive(buffer, offset, size, SocketFlags.None, null, null),
                _socket.EndReceive);
        }
        catch (Exception e)
        {
            if (e is SocketException se && se.SocketErrorCode == SocketError.ConnectionReset)
            {
                throw new ConnectionClosedException("Connection reset");
            }

            throw new ConnectionErrorException("Failed to receieve message", e);
        }

        if (bytesReceived == 0)
        {
            throw new ConnectionClosedException("Connection closed");
        }

        return bytesReceived;
    }

    public async Task<int> SendAsync(byte[] buffer, int offset, int size)
    {
        try
        {
            return await Task<int>.Factory.FromAsync(
                _socket.BeginSend(buffer, offset, size, SocketFlags.None, null, null),
                _socket.EndSend);
        }
        catch (Exception e)
        {
            throw new ConnectionErrorException("Failed to send message", e);
        }
    }

    public void Close()
    {
        _socket.Close();
    }
}

EDIT: Maybe I'll highlight some parts I'm concerned about. Am I doing the Task.Factory.FromAsync right? Especially for ReceiveAsync and SendAsync? I saw a strange example that looked like this:

  var revcLen = await Task.Factory.FromAsync(
                     (cb, s) => clientSocket.BeginReceive(prefix, 0, prefix.Length, SocketFlags.None, cb, s),
                     ias => clientSocket.EndReceive(ias),
                     null);

Why the extra lambda expression? It seems like they are just forcing the usage of one overload while another would be better suited.

Also, do you guys think I should just return zero bytes from ReceiveAsync instead of throwing ConnectionClosedException? Then I can just let the exceptions flow from the socket methods and wrap them in a higher layer. I have a connection abstraction that handles message framing and eventually keep alives.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Just should you not be aware, NetworkStream has ReadAsync and WriteAsync methods. \$\endgroup\$ – VisualMelon Oct 9 '17 at 15:18
  • 2
    \$\begingroup\$ Socket implements IDisposable therefore your class should as well and properly implement the Disposable pattern. \$\endgroup\$ – Jesse C. Slicer Oct 9 '17 at 17:56
3
\$\begingroup\$

Thanks for the suggestions! They helped me make some improvements and directed me towards a better design. I know this is a pretty simple piece of code but still. I moved the error handling and closed connection detection to a higher level connection abstraction. I also added an interface to make mocking and unit testing my message framing and keep alive messages easier. Final implementation below.

EDIT: Mocking this is a massive pain. Hrmm.

public interface ISimpleSocket : IDisposable
{
    void Bind(int port);
    void Listen(int backlog);
    Task<ISocket> AcceptAsync();
    Task ConnectAsync(string host, int port);
    Task<int> ReceiveAsync(byte[] buffer, int offset, int count);
    Task SendAsync(byte[] buffer, int offset, int count);
}

public class SimpleSocket : ISimpleSocket
{
    private readonly Socket _socket;

    private SimpleSocket(Socket socket)
    {
        _socket = socket;
    }

    public SimpleSocket()
    {
        _socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    }

    public void Bind(int port)
    {
        var endPoint = new IPEndPoint(IPAddress.Any, port);
        _socket.Bind(endPoint);
    }

    public void Listen(int backlog)
    {
        _socket.Listen(backlog);
    }

    public async Task<ISimpleSocket> AcceptAsync()
    {
        var socket = await Task.Factory.FromAsync(_socket.BeginAccept, _socket.EndAccept, true);
        return new SimpleSocket(socket);
    }

    public async Task ConnectAsync(string host, int port)
    {
        await Task.Factory.FromAsync(_socket.BeginConnect, _socket.EndConnect, host, port, null);
    }

    public async Task<int> ReceiveAsync(byte[] buffer, int offset, int count)
    {
        using (var stream = new NetworkStream(_socket))
        {
            return await stream.ReadAsync(buffer, offset, count);
        }
    }

    public async Task SendAsync(byte[] buffer, int offset, int count)
    {
        using (var stream = new NetworkStream(_socket))
        {
            await stream.WriteAsync(buffer, offset, count);
        }
    }

    public void Dispose()
    {
        _socket?.Dispose();
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.