8
\$\begingroup\$

I saw this question "Is this implementation of an Asynchronous TCP/UDP Server correct" and it is very similar to what I want to do but it goes about it in a different way so I am wondering if anyone would mind doing some constructive critism on my method please.

I'm looking for any feedback along the lines of "when x happens your server will fall over because of y" or something like that.

My intention is that the server should log out errors that happen and raise an event when it has data. I want it to be really resilient and just deal with errors where it can and carry on waiting for clients but there is a point where I think that is not possible which is why I have the FatalError event.

So, my server class will have this interface:

public interface IMySocketServer
{
    // Start the server listening and return immediately 
    void Open();

    // Stop the server
    void Close();

    // Raised when data is receieved
    event EventHandler<SocketDataEventArgs> DataReceived;

    // Raised when a fatal error has happened such that the
    // server cannot recover itself
    event EventHandler<EventArgs> FatalError;
}

and for completeness here is the event arguments class:

public class SocketDataEventArgs : EventArgs
{
    private readonly byte[] _data;

    public SocketDataEventArgs(byte[] data)
    {
        _data = data;
    }

    public byte[] GetData()
    {
        return _data;
    }
}

My implementation looks like this:

public class MySocketServer : IMySocketServer
{

    private readonly TcpListener _listener;
    private ILogger _logger = NullLogger.Instance;

    public event EventHandler<SocketDataEventArgs> DataReceived;

    public event EventHandler<EventArgs> FatalError;

    public MySocketServer(int port) : this(port, 2048)
    {
    }

    public MySocketServer(int port, int inputBufferSize)
    {            
        InputBufferSize = inputBufferSize;
        _listener = new TcpListener(IPAddress.Any, port);
    }

    public ILogger Logger
    {
        get { return _logger; }
        set { _logger = value; }
    }

    public int InputBufferSize { get; set; }

    protected virtual void OnDataReceived(SocketDataEventArgs e)
    {
        var evt = DataReceived;
        if (evt != null)
        {
            evt(this, e);
        }
    }

    protected virtual void OnFatalError(EventArgs e)
    {
        var evt = FatalError;
        if (evt != null)
        {
            evt(this, e);
        }
    }

    public void Close()
    {
        _listener.Stop();
    }  

    // Start the server - this will throw a SocketException or an 
    // ObjectDisposedException if it cannot start - the caller can catch
    // these and deal with them.  It will also check the buffer is a
    // sensible size and throw an ArgumentOutOfRangeException if not
    public void Open()
    {
        if (InputBufferSize <= 0)
        {
            throw new ArgumentOutOfRangeException("The buffer must be larger than 0");
        }

        _listener.Start();

        var res = _listener.BeginAcceptSocket(OnAcceptSocket, _listener);
        if (res.CompletedSynchronously)
        {
            AcceptSocket(res);
        }
    }

    private void OnAcceptSocket(IAsyncResult res)
    {
        var thread = Thread.CurrentThread;
        if (string.IsNullOrEmpty(thread.Name))
        {
            thread.Name = "Accept " + thread.ManagedThreadId;
        }

        if (!res.CompletedSynchronously)
        {
            AcceptSocket(res);
        }
    }

    private void AcceptSocket(IAsyncResult res)
    {

        // End the current Accept and get the socket and start another
        // one straight away so as not to block the server
        var listener = (TcpListener)res.AsyncState;

        Socket socket = null;
        try
        {
            socket = listener.EndAcceptSocket(res);
        }
        catch (ObjectDisposedException e)
        {
            // Will log this exception but I do not think there is anything
            // else to do here?
            Logger.Error(e);
        }
        catch (SocketException e)
        {
            // If the socket is closed before we can end log the error but
            // bury it since it is probably a client that has disconnected
            Logger.Error(e);
        }

        try
        {
            var r = listener.BeginAcceptSocket(OnAcceptSocket, listener);
            if (r.CompletedSynchronously)
            {
                AcceptSocket(r);
            }
        }
        catch (SocketException e)
        {
            // This is really bad if this happens - we will not
            // be able to accept the next TCP request not sure there
            // is much I can do here, the server is just broken, or
            // is there something I can do??
            Logger.Error(e);
            OnFatalError(new EventArgs());
        }
        catch (ObjectDisposedException e)
        {
            // See above - same reason
            Logger.Error(e);
            OnFatalError(new EventArgs());
        }

        if (socket != null)
        {
            // If it gets here, we are waiting for the next client and we
            // have the socket for this client
            HandleSocket(socket);
        }
    }                    

    private void HandleSocket(Socket socket)
    {
        var ms = new MemoryStream();
        using (socket)
        {
            try
            {
                // Read the input in chunks and append them
                // to the data array
                var buffer = new byte[InputBufferSize];
                int read;
                while ((read = socket.Receive(buffer,
                                              0,
                                              buffer.Length,
                                              SocketFlags.None)) > 0)
                {
                    ms.Write(buffer, 0, read);
                }
            }
            catch (SocketException e)
            {
                Logger.Error(e);
            }
        }

        // Now everything should be closed we can raise the data received
        // event
        OnDataReceived(new SocketDataEventArgs(ms.ToArray()));
    }
}

Update

It has been pointed out that I should clarify some thnigs (thanks Bobby):

  1. The names of the types are just because I didn't want to say exactly what I would call it in real life since it would describe the product (rest assured the name is not My anything and is wonderfully descriptive).

  2. I didn't go XML doc comment style because I wanted to keep the code part brief for this question (I do always xml comment in the real world, but I wanted to know about the implementation and just commented essentially what each bit is doing).

  3. The Data of my SocketEventArgs should not be a readonly property because I do not want it to be modified by the caller.

  4. I don't agree with the naming convention for c# member variables described at Guidelines for Names as I have found that I really do want to know if a variable is a member) and the _ is pretty common (and less tedious and mistake prone than requiring this.) - just a personal preference thing I know!

  5. The wrapping is because I keep the code to 80 characters wide (this is a convention to make it easy to read on whatever resolution/font size you are using - I find horizontal scrolling code to be a horrible reading experience) - lining up the parameters like that is so you can see which method they are gonig into.

\$\endgroup\$
  • \$\begingroup\$ No comment on the listener at the mo, but a quick suggestion would be to make Logger private unless there's a specific reason you want to expose it to the world. I would then set it via the constructor. \$\endgroup\$ – dreza Jul 18 '12 at 8:17
  • \$\begingroup\$ The logger is like that so that it can be injected by an IoC container (if an ILogger is registered with the container, if not it will use the NullLogger) \$\endgroup\$ – kmp Jul 18 '12 at 10:13
  • \$\begingroup\$ Modules like Ninject and Unity allow injection via constructor. However if it's a design need, then fair enough. I like to remove as much public exposure of properties as I can if it's not needed that's all. \$\endgroup\$ – dreza Jul 18 '12 at 10:26
  • \$\begingroup\$ Yup, I do not know about those two containers but I use Windsor and a constructor argument that is not registered with the container will result in the class not being able to be resolved (here I don't care whether ILogger is registered or not - it is an optional dependency) - good point about the public exposure thing but in this case I think it is justified \$\endgroup\$ – kmp Jul 18 '12 at 11:11
3
\$\begingroup\$

I'd raise the events DataReceived and FatalError on a different thread, to avoid blocking the TCP work. To elaborate this point, I suggest you take a look at Kayak's implementation (it's an implementation of HTTP server, but the core looks quite similar).

Other than that, any magic number, like the 1024 bytes buffer size, should be exposed somehow, or at least be injected in the constructor.

Update

You use a setter to the buffer size without validation. I'd throw an exception if the new value is non-positive.

In your HandleSocket method, you build a byte array in a non-effective technique: you reallocate memory in each iteration. I'd use a List<byte> and invoke its AddRange method. After the loop is done - invoke ToArray.

As for the async pattern, it looks non-blocking what so ever, I missed the point of handling the socket after re-invoking BeginAcceptSocket.

\$\endgroup\$
  • \$\begingroup\$ Thanks very much! I have added the buffer size as a constructor argument (via an overload to make it optional and a property) and will have a look at the Kayak stuff about raising the events in a separate thread - as I am using the asynchronous pattern though - will it actually block the TCP work as it stands? \$\endgroup\$ – kmp Aug 20 '12 at 5:20
  • \$\begingroup\$ Thanks for your update (sorry, I didn't notice it) - I added some validation into Open to check the buffer size (I dunno, I just prefer it if setters and constructors don't throw - rather have Open check the state of the world before it starts off). About the array copy stuff: I thought that was a bit poor but the thing is I cannot call AddRange on a List<byte> because it only takes an IEnumerable and would, therefore, append the entire buffer, but I want to append just the "read" bytes. I can only thing a for loop and calling Add on the List would work, but would that be any better? \$\endgroup\$ – kmp Sep 6 '12 at 8:02
  • \$\begingroup\$ I asked about the array stuff over on Stack Overflow: stackoverflow.com/q/12296053/1039947 and went with a MemoryStream. \$\endgroup\$ – kmp Sep 6 '12 at 8:59

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.