Simple TCP Client Server application with receive side only

Question

I wrote simple TCP Client Server application, I was wonder could you please review my code. I would be grateful for any hints and tips regarding to logic of app, coding style or naming convention.

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Diagnostics;
using System.Net;
using System.Net.Sockets;
using System.Threading;

namespace TcpConnectionTest
{
    class Program
    {

        private static void handleTcpConnectionEvents(object sender, TcpConnectionEventArgs args)
        {
            string s = null;

            switch (args.EventType)
            {
                case TcpConnectionEventType.ServerIsListening:
                    s = "TCP Server listens ...";
                    break;
                case TcpConnectionEventType.ConnectionIsEstablished:
                    s = "Connection has been established";
                    break;
                case TcpConnectionEventType.ClienetIsDisconnected:
                    s = "Client has been disconnected";
                    break;
                case TcpConnectionEventType.ServerStoppedListening:
                    s = "TCP Server stopped listenining";
                    break;
                case TcpConnectionEventType.ByteReceived:
                    s = String.Format("{0:X2}", args.ByteReceived);
                    break;
            }

            Console.WriteLine(s);
        }

        static void Main(string[] args)
        {
            Console.WriteLine("Program run in Console mode!");
            Console.WriteLine("Press ENTER to to exit ...");

            try
            {
                TcpConnection.Instance.TcpConnectionEvent += new EventHandler<TcpConnectionEventArgs>(handleTcpConnectionEvents);
                TcpConnection.Instance.Start(new TcpListener(new IPAddress(new byte[] { 127, 0, 0, 1 }), 3000));
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
            }

            while (true)
            {
                ConsoleKeyInfo info = Console.ReadKey();

                if (info.Key == ConsoleKey.Enter)
                {
                    TcpConnection.Instance.Stop();
                    Console.WriteLine("Good Bye!");
                    Thread.Sleep(2000);
                    break;
                }
            }
        }
    }
}

TcpConnection.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Net;
using System.Net.Sockets;
using System.Diagnostics;

namespace TcpConnectionTest
{
    public enum TcpConnectionEventType
    {
        ServerIsListening,
        ConnectionIsEstablished,
        ClienetIsDisconnected,
        ServerStoppedListening,
        ByteReceived,
    }

    public class TcpConnectionEventArgs : EventArgs
    {
        public TcpConnectionEventArgs(TcpConnectionEventType type)
        {
            EventType = type;
        }

        public TcpConnectionEventArgs(byte b)
        {
            EventType = TcpConnectionEventType.ByteReceived;
            ByteReceived = b;
        }

        public TcpConnectionEventType EventType { get; set; }
        public byte ByteReceived { get; set; }
    }

    public class TcpConnection
    {
        private static TcpConnection instance = null;
        private volatile bool _reqStopListen = true;
        private static object syncLock = new object();
        private readonly object _stopListenLock = new object();
        private TcpListener _tcpListener = new TcpListener(new IPAddress(new byte[] { 127, 0, 0, 1 }), 3000);
        private TcpClient _tcpClient = null;
        private Thread _tcpSlaveThread = null;

        TcpListener Listener
        {
            get { return _tcpListener; }
            set { _tcpListener = value; }
        }

        TcpClient Client
        {
            get { return _tcpClient; }
            set { _tcpClient = value; }
        }

        Thread TcpSlaveThread
        {
            get { return _tcpSlaveThread; }
            set { _tcpSlaveThread = value; }
        }

        private bool ReqStopListen
        {
            get
            {
                lock (_stopListenLock)
                {
                    return _reqStopListen;
                }
            }
            set
            {
                lock (_stopListenLock)
                {
                    _reqStopListen = value;
                }
            }
        }

        private TcpConnection()
        {
        }

        public static TcpConnection Instance
        {
            get
            {
                lock(syncLock)
                {
                    if (TcpConnection.instance == null)
                    {
                        TcpConnection.instance = new TcpConnection();
                    }
                    return TcpConnection.instance;
                }
            }
        }



        private void AcceptCompleted(IAsyncResult ar)
        {
            Socket socket = null;

            try
            {
                socket = Listener.Server.EndAccept(ar);
                Client = new TcpClient { Client = socket };

                TcpSlaveThread = new Thread(ThreadTcpSlave);
                TcpSlaveThread.Start();

                if (TcpConnectionEvent != null)
                {
                    TcpConnectionEvent(this, new TcpConnectionEventArgs(TcpConnectionEventType.ConnectionIsEstablished));
                }
            }
            catch (Exception e)
            {
                Debug.Print(e.Message);
            }
        }
        public EventHandler<TcpConnectionEventArgs> TcpConnectionEvent;

        static void ThreadTcpSlave()
        {
            while (Instance.ReqStopListen)
            {
                byte[] data = new byte[1];

                try
                {
                    if (Instance.Client.Client.Poll(50, SelectMode.SelectRead))
                    {
                        Instance.Client.Client.Receive(data, 0, 1, SocketFlags.None);

                        if (Instance.TcpConnectionEvent != null)
                        {
                            Instance.TcpConnectionEvent(Instance, new TcpConnectionEventArgs(data[0]));
                        }
                    }
                }
                catch (Exception e)
                {
                    Debug.Print(e.Message);
                    continue;
                }
            }
        }



        public void Start(TcpListener listener)
        {
            Listener = listener;
            Listener.Start();
            Listener.Server.BeginAccept(AcceptCompleted, Listener);

            if (TcpConnectionEvent != null)
            {
                TcpConnectionEvent(this, new TcpConnectionEventArgs(TcpConnectionEventType.ServerIsListening));
            }
        }

        public void Stop()
        {
            if (Listener != null)
            {
                Listener.Server.Close();
                if (TcpConnectionEvent != null)
                {
                    TcpConnectionEvent(this, new TcpConnectionEventArgs(TcpConnectionEventType.ServerStoppedListening));
                }
                Listener = null;
            }

            if (TcpSlaveThread != null && TcpSlaveThread.IsAlive)
            {
                ReqStopListen = false;
                Instance.TcpSlaveThread.Join(new TimeSpan(0, 0, 0, 100));
            }
        }
    }
}

Answer 1

Lets start with the singleton TcpConnection

1. Singletons are usually anti-patterns but if are so determined about them, then you don't need to introduce a lock to insure thread-safety. Simply initialize it statically.

   private static TcpConnection instance = new TcpConnection();
   

If you want laziness then you have to use a nested class that has a static constructor

    public static TcpConnection Instance { get { return Nested.instance; } }
    private class Nested
    {
    
    static Nested()
    {
    }

    internal static readonly TcpConnection instance = new TcpConnection();
    }

2. THREADS ARE EVIL, don't touch them and use Tasks instead. Maybe you don't know that threads are VERY EXPENSIVE resources. Avoid them, they are really hard to get right

   private Task _tcpSlaveTask;
   
   _tcpSlave = Task.Factory.StartNew(()=> {...});
   

3. NEVER write Thread.Sleep(2000), use Task.Delay(2000) instead if you ever need to do that. This is a magic number that worked on your machine but this doesn't mean that it's that right number for the production machine.

4. Avoid this shared-state so you can avoid this volatile field, look what Eric Lippert says about volatile:

Frankly, I discourage you from ever making a volatile field. Volatile fields are a sign that you are doing something downright crazy: you're attempting to read and write the same value on two different threads without putting a lock in place.

Check this out for more details

I don't know if that's a production code or not, if it is, then consider rewriting it NOW and use the TPL library instead

5. Fields of reference types are set to null and you don't need to do that explicitly

   private TcpClient _tcpClient; // this is null