Memory Leak with TCPClient in separate thread

Question

I'm trying to read some string data from a Network stream with the following functions:

static TcpClient client;
static NetworkStream nwStream;

private void conn_start_Click(object sender, EventArgs e)
{
   //Click on conn_start button starts all the connections

   client = new TcpClient(tcp_ip.Text, Convert.ToInt32(tcp_port.Text));
   nwStream = client.GetStream();
   readBuff="";
   Timer.Start();
}

string readBuff;
private void readFromConnection()
{
  string x1 = "";
  byte[] bytesToRead = new byte[client.ReceiveBufferSize];
  int bytesRead = nwStream.Read(bytesToRead, 0, client.ReceiveBufferSize);
  x1 = Encoding.ASCII.GetString(bytesToRead, 0, bytesRead);
  //
  //some more code to format the x1 string      
  //
  readBuff = x1;

  bytesToRead = null;
  GC.Collect();
}

Now the readFromConnection() function is called every second from a Timer Tick event with the following code:

private void Timer_Tick(object sender, EventArgs e)
{
   Thread rT1 = new Thread(readFromConnection);
   rT1.Start();
   //app crashes after 40-45 min, out of memory exception.
}

This is causing some memory leak. The application crashes with OutOfMemory exception after running 40-45 mins.

My question is if there is proper way to handle the new thread, as it will be alive for 1 second only? How can I overcome this issue?

I have to run this function in a new thread as when on the same thread as the UI, it tends to freeze it. Even a small mouse movement takes seconds to get processed.

In the same perspective, if the Tick event calls the function in the same thread, there is no such issue of memory leak.

private void Timer_Tick(object sender, EventArgs e)
{
   readFromConnection();
   //No memory leak here.
}

I've couple questions for you, Why are you calling `GC.Collect()` manually? and why are you creating threads inside `Timer` Tick Event? — Rickless, Dec 11 '17 at 18:24
I added the GC to check if it solves any of the memory leak. Initially it wasn't there. For the new thread, it was done to to prevent the main UI from freezing. Please let me know if it is a bad approach to do so? — chiro3110, Dec 11 '17 at 18:35
You should not call `GC.Collect()` manually, the garbage collection is managed for you. You can get rid of the timer by wrapping your logic inside an infinite while loop, and adding a sleep instruction between each iteration, this way, you end up with one thread only. — Rickless, Dec 11 '17 at 18:37

score 3 · Accepted Answer · answered Dec 11 '17 at 18:51

No need for the Timer, you can put your Receive logic inside one infinite while loop, and you can add a sleep instruction for the thread between each iteration. You shouldn't use ReceiveBufferSize to read data from socket stream. ReceiveBufferSize is not the same as how many bytes the other party has sent or how many bytes are available for me to read. You can use Available instead even though I don't trust this property either when I read a large file from the network. You can use client.Client.Receive method, this method will block the calling thread.

private void readFromConnection()
{
    while (true)
    {
        if(client.Connected && client.Available > 0)
        {
            string x1 = string.Empty;
            byte[] bytesToRead = new byte[client.Available];
            int bytesRead = client.Client.Receive(bytesToRead);
            x1 = System.Text.Encoding.Default.GetString(bytesToRead);
        }
        Thread.Sleep(500);
    }
}

Finally, regarding the GC take a look at this question

Thanks for the improvement suggests. All of them worked good! — chiro3110, Dec 12 '17 at 17:28

score 1 · Answer 2 · answered Dec 11 '17 at 18:25

Just use Flush the memory stream to prevent memory leak.

private void readFromConnection()
{
  string x1 = "";
  byte[] bytesToRead = new byte[client.ReceiveBufferSize];
  int bytesRead = nwStream.Read(bytesToRead, 0, client.ReceiveBufferSize);
  x1 = Encoding.ASCII.GetString(bytesToRead, 0, bytesRead);
  //
  //some more code to format the x1 string      
  //
  readBuff = x1;

  bytesToRead = null;
  nwStream.Flush(); // Add this line
  GC.Collect();
}

Also, using Timer and trigger readFromConnection is bad approach. Simply, use a while statement to read stream.

the Flush did not make any change to the memory leak, but the suggestion of changing the approach helped. Thanks! — chiro3110, Dec 12 '17 at 17:27

Memory Leak with TCPClient in separate thread

2 Answers2