2
\$\begingroup\$

It's my first stab at TCP/IP and honestly I'm just starting out on C# also, so any comments on the design, method, etc are more than welcome.

Basically I wanted to create a listening thread and someday a sending thread, that can handle listen/send heartbeat message and also other messages.

Once it retrieve the message, it will update some value back into the main execution program.

public virtual void StartReceivingThread()
{
    Thread thrReceive = new Thread(Receive);
    try
    {
        if (!bIsActive && Connect())
        {
            //NOTE: exception thrown by a thread can only be captured by that thread itself
            //start a listen thread
            //wait until heartbeat message is accepted

            thrReceive.Name = "thr" + serviceType.Name;
            thrReceive.Start();
            bIsActive = true;

            //wait to get the heartbeat message
            for (int i = 0; i < maxRetry; i++)
            {
                Thread.Sleep(maxTimeOutValue);
                if (bIsReceivingHeartbeat)
                    break;
            }
            //if nothing happens close the connection and try again
            if (!bIsReceivingHeartbeat)
            {
                bIsActive = false;
                CleanUp();
                logger.Info("Closing  receiver thread - " + thrReceive.Name);
            }
            else
            {
                logger.Info("Starting  receiver thread - " + thrReceive.Name);
            }
        }


    }
    catch(Exception ex)
    {
        logger.Error(ex);
    }
    //finally
    //{
    //    logger.Info("Exiting  receiver thread - " + thrReceive.Name);
    //}
}

public void CleanUp()
{
    if (client != null)
    {
        client.Close();
    }
}

public virtual void Receive()
{
    string eventMessage = string.Empty;
    int bytesRcvd = 0;
    int totalBytesRcvd = 0;
    byte[] byteBuffer = new byte[maxBufferSize];
    NetworkStream listenStream;
    try
    {
        if (client.Connected)
        {
            listenStream = client.GetStream();    
        }
        else
        {
            return;
        }

        while (true)
        {                
            //message that is slot in from the object will get sent here.
            if (!string.IsNullOrEmpty(MessageToSend))
            {
                Send(MessageToSend);
                MessageToSend = string.Empty;
            }

            // must convert it back and look for the delimiter, cannot wait for the three heartbeat to pass
            string leftoverMsg = string.Empty;

            bytesRcvd = listenStream.Read(byteBuffer, totalBytesRcvd, maxBufferSize - totalBytesRcvd);
            totalBytesRcvd += bytesRcvd;

            //if more than heart beat size, can process to see if it's a heartbeat and proceed to send
            if (totalBytesRcvd > msgHeartbeatSize)
            {
                eventMessage = Encoding.ASCII.GetString(byteBuffer, 0, totalBytesRcvd);
                ProcessMessage(eventMessage, ref leftoverMsg, ref totalBytesRcvd, ref byteBuffer);
            }
        }
    }
    catch (ThreadAbortException thEx)
    {
        //do nothing as main thread has aborted and waiting to close
        logger.Info(Thread.CurrentThread.Name + " is stopped. ");
    }
    catch (Exception exce)
    {
        bIsActive = false;
        logger.Error(exce);
        CleanUp();
    }
    finally
    {
        logger.Info(String.Format("Thread {0} Exiting. ", Thread.CurrentThread.Name));
    }
}

public void Send(string msg)
{
    StringBuilder sb = new StringBuilder();
    sb.Append(Char.STX);
    sb.Append(msg);
    sb.Append(Char.ETX);

    try
    {
        string message = sb.ToString();
        byte[] byteBuffer = Encoding.ASCII.GetBytes(message);

        //use a different stream to prevent blocking?
        NetworkStream sendNetStream = client.GetStream();
        sendNetStream.Write(byteBuffer, 0, byteBuffer.Length);
        sendNetStream.Flush(); //clear buffer straight without waiting for the write buffer to fill up.

        //simplify the heart beat message for logging
        if (simpleHeartbeatMsg.ToLower().Contains("on") 
            && message.Contains("fnxheartbeatack"))
        {
            message = "heartbeat message.";
        }
        logger.Info("Sending to   - xml: " + Char.GetNonFormattedString(message));

    }
    catch (Exception ex)
    {
        bIsActive = false;
        CleanUp();
        sb = null;
        throw ex;
    }
}
\$\endgroup\$
3
\$\begingroup\$

A few things come to mind when reading this...

  • Set your thread priority. Remember that spawned threads do not follow the priority of the host process. So you need to decide what level this should be relative to other priorities
  • This is going to be a foreground thread, is that really what you want here. In other words, when the host process comes down, should this thread hang the process until your abort it? Consider the background option
  • Using a Spin-Wait pattern with Sleep may not be the best way to go. Consider using a Monitor.Pulse/Wait pattern, you could even set a Wait timeout if needed.
  • If the logger is Log4Net, make sure to use the if (logger.IsInfoEnabled()) pattern which is recommended by the project owner. This will offset some internal expenses.
  • The Cleanup() method is not visible in your post, but this is called inside the catch block. Make sure this cannot throw an exception also
  • Pretty much all your code is inside a try-catch block. In doing so, none of it will be eligible for JIT Optimizations. Depending on your performance needs, that could be a problem. Move the insides of the try section into seperate methods
  • Given your use of the StringBuilder it will cost 2 or 3 times more then just doing an x+y or string.concat(x, y). Write a performance test around these and your will see what I mean
  • Using ToUpper() vs ToLower() is more performant
  • Your not Closing or Disposing your NetworkStream

Hope this helps

\$\endgroup\$
0
\$\begingroup\$

One thing about the heartbeat timeout on the receiving end. Sleeping and polling is generally an indicator that you're not doing asynchronous I/O correctly. In this case, you're better off starting a countdown timer and resetting it every time you receive a heartbeat. If the timer expires, that indicates a timeout. It'll be more accurate and allow for better threading practices.

\$\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.