Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm working on a socket application that will listen to incoming messages from about 400 devices. They all send a message at least once per minute. Now I have a working program and it has been running for a few days without problems as a test with about 10 devices, but I keep thinking I missed something and it could fail at one point for some reason.

Now below is a stripped version of the code. It listens for clients and saves the raw data it receives to a database. It is important that it runs all the time.

Overall question: Excluding external factors like loss of internet connection/power, could this program run indefinitely in the current version?

My main concerns:

  • Missing an occasional message is acceptable. Is it wise to have a try..catch around most of the code that just logs the error and continue? I know I should handle errors case by case, but since I have no idea what to expect, how can I handle it?
  • Once started, could the _tcpListener stop listening for some reason or crash the while loop?
  • I have the _db on the main thread and handle all updates there. Is this better then a new db connection for each thread? (e.g.: using (var db = new entity) { updatedb })
  • If we would upscale to more devices (e.g.: 1000 or even 5000), what would eventually be the downfall?

The code shows I just save the raw data, but there are actually about 7 types of messages. The form contains 7 listboxes, one for each type. I want to show the last 1000 messages for each type. But I would also like to be able to filter the last 1000 records based on an input field. This part is handled in the Updatelist method.

Although this works, it just feels wrong. I have to update the entire list every time a message is received. Is there a better way of doing this?

namespace MyCommunication
{
  public partial class Form1 : Form
  {
    private const int MaxLengthList = 1000;
    private readonly List<string> _listRaw = new List<string>();

    private MyEntities _db;
    private TcpListener _tcpListener;
    private Thread _listenThread;

    public Form1()
    {
      InitializeComponent();

      if (InitServer())
          StartServer();
    }

    delegate void UpdatelistDelegate(ListBox box, List<string> list, string message);
    private void Updatelist(ListBox box, List<string> list, string message)
    {
      if (InvokeRequired)
      {
        Invoke(new UpdatelistDelegate(Updatelist), new object[] { box, list, message });
        return;
      }

      if (!string.IsNullOrEmpty(message))
      {
        list.Add(message);
        while (list.Count() > MaxLengthList)
          list.RemoveAt(0);
      }

      //filter input field.
      if (tbFilter.Text.Length > 0)
      {
        var filter = new List<String>();
        filter.AddRange(list.Where(e => e.Contains(tbFilter.Text)));
        box.DataSource = null;
        box.DataSource = filter;
      }
      else
      {
        box.DataSource = null;
        box.DataSource = list;
      }
    }

    delegate void UpdateRawDataDelegate(RawData data);
    private void UpdateRawData(RawData data)
    {
      if (data == null) return;
      if (InvokeRequired) { Invoke(new UpdateRawDataDelegate(UpdateRawData), new object[] { data }); return; }

      try
      {
        _db.RawData.Add(data);
        _db.SaveChanges();

       //example of update listbox
       Updatelist(lbRawData, _listRaw, "Added rawdata");
      }
      catch (Exception ex)
      {
        //log error
        UpdateError(ex);
      }

    }

    private Boolean InitServer()
    {
      try
      {
        _db = new MyEntities();
        _tcpListener = new TcpListener(IPAddress.Any, 5000);
        _listenThread = new Thread(ListenForClients);
      }
      catch (Exception ex)
      {
        //log error
        UpdateError(ex);
        return false;
      }

      return true;
    }

    private void StartServer()
    {
      _listenThread.Start();      
    }

    private void ListenForClients()
    {
      try
      {
        _tcpListener.Start();

        while (true)
        {
          var client = _tcpListener.AcceptTcpClient();
          var clientThread = new Thread(HandleClientComm);
          clientThread.Start(client);
        }
      }
      catch (Exception)
      {
        //log error
        UpdateError(ex);
      }
    }

    private void HandleClientComm(object client)
    {     
      var tcpClient = (TcpClient)client;
      var clientStream = tcpClient.GetStream();

      var received = new byte[966]; //966 max length of message
      int bytesRead = 0;

      try
      {
        bytesRead = clientStream.Read(received, 0, 966);
      }
      catch (Exception ex)
      {
        UpdateError(ex);
      }

      if (bytesRead <= 16)
      {
        tcpClient.Close();
        return; //nothing to see here (needs atleast 16 bytes for header and footer)
      }

      var data = new RawData
      {
        DateFormat = DateTime.Now,
        Data = received
      };

      UpdateRawData(data);

      try
      {
        //do whatever needs to be done with the received message
      }
      catch (Exception ex)
      {
        //log error
        UpdateError(ex);
      }

    }

  }
}
share|improve this question

4 Answers 4

up vote 2 down vote accepted

Each time your server receives a message, it creates a new thread to handle the message and then ends the thread after handling one message. This is not scalable, because creating and ending threads is so expensive.

IMHO, a better approach would be to have a thread that listens for messages and places them on a queue. Create and maintain a pool of threads which pops messages off the queue and deals with them.

The total number of threads should be equal to the number of CPU cores, so that the OS does not waste time context switching between threads.

You will need to monitor the queue length. If it start to grow, then your server is being overwhelmed and you will need to execute some way of reducing the workload.

Remember that the pool of worker threads needs to access the queue so that only one thread can access the queue at a time. Some frameworks provide a thread safe queue ( very cool! ) but if you do not have one of these you will need to protect the queue with a mutex or similar - don't try to create your own thread safe queue ( that is a huge challenge )


A more detailed problem that I believe exists in your code: message framing. As your server becaomes busy, messages will begin to arrive together and your read will start reading more than one message at a time. You can also expect that you will get fractions of the next message being read along with the previous message. I do not see any code to deal with this. Your message protocol needs to have an unambiguous marker of where messages start and end, and you need code to repair framing errors.

In my experience, even if you have just one client and one server, if you run the connection anywhere near its bandwidth, you will start to get framing errors. In this case, with multiple devices sending asynchronous messages, you will get framing errors from time to time with even moderate loads. It is best to be always ready to handle them, because they never show up in trials, but then when your code gets out into the real world ...

Visualize this:

  1. A message arrives from one of your devices

  2. You code asks for a new thread to be created - with the result a lot of thrashing around, memory allocation and context switching begins.

  3. Meanwhile another message arrives

  4. Finally, the new thread is ready to read ... and gets more than the single message expected.

share|improve this answer
    
Hi, and welcome to Code Review. I disagree with your claim that there's a framing problem. Each client has their own socket in their own thread... no mixing of packets/framing. I suspect the comment about the expensive thread creation as well. Is there some evidence of that? (Related: Why so much difference in performance between Thread and Task?) –  rolfl Jun 25 at 16:13
    
@rofli I will try to explain the framing problem in my answer - these comments are way too constricted. –  ravenspoint Jun 25 at 16:35
    
ravenspoint - let's continue this discussion in chat -> The 2nd Monitor –  rolfl Jun 25 at 17:03
    
I think you have a misunderstanding about how C# sockets work. The 'accept' call var client = _tcpListener.AcceptTcpClient(); creates a new client-socket instance, which only ever receives data from that particular remote endpoint. The network framing issues are all handled much lower in the stack than the C# code. The stream that is 'pulled' from that client Socket tcpClient.GetStream(); only ever contains data from one client.... always. –  rolfl Jun 25 at 17:14
1  
re the 'asynchronous server': I have not looked at the code you posted in detail. Some general comments are: your code is already doing asynchronous ( non-blocking ) reads. However, this is unnecessary and wasteful in your case because all your messages are arriving on one network card ( I assume ) and are therefore serialized by the hardware ( only one can be read at a time ). What people usually mean by an 'asynchronous server' is that the wait for a new message is non-blocking. This is important when there is a GUI that needs to be responsive, but not in your case, AFAIK –  ravenspoint Jun 26 at 12:17

I got a few comments on your code. First of all, try to separate your UI code from your backend code.

You should be using the using statement here

private void HandleClientComm(object client)
{     
  var tcpClient = (TcpClient)client;
  using(var clientStream = tcpClient.GetStream()){
   // code goes here
 }
}

Always try to put your constants in constant variables

var received = new byte[966]; // dont do that

use constants instead

private const int messageLength  = 966;

and use it afterwards

var received = new byte[messageLength];

There is some redundant code

if (tbFilter.Text.Length > 0)
  {
    var filter = new List<String>();
    filter.AddRange(list.Where(e => e.Contains(tbFilter.Text)));
    box.DataSource = null;
    box.DataSource = filter;
  }
  else
  {
    box.DataSource = null; // no need for that 
    box.DataSource = list;
  }

can be reworked for this

  if(tbFilter.Text.Length > 0)
  {
    list  = list.Where(e => e.Contains(tbFilter.Text));
    box.DataSource = null;
  }
  box.DataSource = null; // I doubt thats needed
  box.DataSource = list;

Consider using a higher abstraction for dealing with concurrency . Task for instance. Threads are very expensive and low level. With Tasks, you can easily report the progress. [Have a look at async programming in .Net 1. You have to be a bit careful when writing concurrent programs.

share|improve this answer
    
could you explain why using is necessary? If you expect load to be high enough that it'd be necessary, it would be worth using completely different I/O paradigm. Also, your reworking of the redundant code isn't completely identical; it changes list whereas the existing code doesn't. –  Snowbody Jun 25 at 14:20
1  
@Snowbody using is used here to make sure that the stream will get closed. in regards to list, I just gave a suggestion, filter could only be used without code redundancy –  Sleiman Jneidi Jun 25 at 14:22
    
See my remark about not seperating code at @unholy's answer. But are threads more expensive then tasks then? I will look at your link, but it feels strange that a low level solution would be more expensive. –  Hugo Delsing Jun 25 at 15:09
    
@HugoDelsing interesting remark, low level solution sometimes is more expensive. Yes, it's often hard to get the low level solution right, higher level abstractions are there to do the job for you :) –  Sleiman Jneidi Jun 25 at 15:12
    
I've read your link about the task, but I cant say i'm convinced I need it here. I feel it would be needed when you have a single long running task that you might need to cancel/monitor. In my case each received message is handled very quickly, I just need to be able to handle more at the same time. –  Hugo Delsing Jun 26 at 6:59

Coupling of UI and business logic:

All of the socket handling is done in private methods of your form class. This makes it very difficult to test without actually starting up the application. Instead, if you move the business logic of processing a message into a different class, you can write automated tests that verify the code does what you want for good and bad messages.


Magic numbers:

You define MaxLengthList as a constant, which is the correct thing to do. However, you don't do this with any of the other values (socket timeout, required bytes, ...).


Exception handling:

If the socket read fails, it is very unlikely that you will be able to continue any further. However, your catch block does not stop the code from executing further.

ListenForClients references ex but does not define the variable.

Even for applications that are meant to be fault tolerant, I don't like many instances of catching Exception. This will catch all exceptions including NullReferenceException and OutOfMemoryException. When you make a call to a specific method, you should know the "normal" exceptions it might throw and you should handle those directly. An IOException when reading from a socket and a NullReferenceException when calling Read() are two very different things and should be treated differently.


Consistent if statements:

Your code has all of the following if formats.

  • One line, no braces for true clause
  • One line, braces for true clause (technically two statements with second being return)
  • Multi line, braces for true clause, two statements on different lines with second being return
  • Multi line, braces for true clause, many statements on different lines
  • Two line, no braces for single line true (style also used for while loops)

Pick one style and be consistent. I prefer to always be multi-line and always include braces. If you need to add a line to a block, the braces are already there and you won't have to worry about the second line executing unconditionally.


There is no way to stop you while(true) loop.

share|improve this answer
    
Thank you. My first thought was to put it into a seperate class, but that makes updating the UI more difficult, because you can't use InvokeRequired. I use the different if statements, because some lines are clear on a single line and I feel using 5 lines causes clutter. –  Hugo Delsing Jun 25 at 15:04
2  
@HugoDelsing: You can pass Updatelist as a delegate to your business logic class. Updatelist can still maintain the InvokeRequired check. –  unholysampler Jun 25 at 15:12

A minor note about TCP. You are not guaranteed to get the whole message at once. You may receive just part of the message -- and then it may take an arbitrary amount of time for you to get the next part (which may or may not complete the message). This is rare but happens consistently as network load increases (when the network detects congestion, automatic throttling occurs and the client doesn't send all the data in one packet). It's not as much of an issue in your case since you've said you can tolerate missing some messages (and hopefully the client can tolerate your slamming the connection shut).

share|improve this answer
    
Good point. My first thought was to read smaller chunks and wait for the rest, but since it worked I assumed it would always work. I will adjust it. –  Hugo Delsing Jun 25 at 15:05
2  
Always ask for the largest size you're prepared to deal with at once, but don't ever expect to get the whole thing. (Also, TCP is a stream, so the underlying OS is allowed to combine packets for efficiency--you may get two messages at once) –  Snowbody Jun 25 at 15:10

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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