producer/consumer get different data by Queue

Question

Solved!!! Thank you

Beware of what kind of object you put into Queue.If you put a value, such as int, then enqueue will make a copy and everyone is happy. If you put a reference, such as byte[], string, enqueue put this reference into queue, then here comes problem. If this reference is changed before consumer read it, the consumer will read changed version of data.

To avoid this issue, get a new version of frame reference right after enqueue, in rxThread. Code:

public void rxThreadFunc()
        {
         byte[] data = new byte[datalen];//declare for the first iteration.
            int j = 0;
            while (true)
            {

                for (int i = 0; i < rxlen; i++)
                {
                    data[j] = (byte)i;
                    j++;
                    if (j >= datalen)
                    {
                        j = 0;
                        mQ.Add(data);
                        using (StreamWriter fwriter = new StreamWriter("C:\\testsave\\rxdata", true))
                        {
                            for (int k = 0; k < datalen; k++)
                            {
                                fwriter.Write(data[k]);
                                fwriter.Write(",");
                            }
                            fwriter.Write("\n");
                        }
                     data = new byte[datalen];//create new reference after enqueue/Add
                    }
                }

            }
        }//rxThreadFunc()

Update1 I just wrote another simpler code so everyone can test it without serialport hardware. click button to run program. I think thread priority causes this problem, without changing rxthread's thread priority the dataProc thread will get right data. but I still don't know why.

rxThread.Priority=ThreadPriority.Hightest

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Collections.Concurrent;
using System.Threading;
using System.IO;
namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();

        }

        private void button1_Click(object sender, EventArgs e)
        {
            Thread rxThread = new Thread(rxThreadFunc);
            rxThread.Priority = ThreadPriority.Highest;//this causes problem
            rxThread.Start();
            Thread procThread = new Thread(dataProc);
            procThread.Start();
        }
        BlockingCollection<byte[]> mQ = new BlockingCollection<byte[]>();
        int datalen = 30;         
        int rxlen = 200;
        public void rxThreadFunc()
        {
            int j = 0;
            while (true)
            {
               byte[] data = new byte[datalen];//is this in the right place?
                for (int i = 0; i < rxlen; i++)
                {
                    data[j] = (byte)i;
                    j++;
                    if (j >= datalen)
                    {
                        j = 0;
                        mQ.Add(data);
                        using (StreamWriter fwriter = new StreamWriter("C:\\testsave\\rxdata", true))
                        {
                            for (int k = 0; k < datalen; k++)
                            {
                                fwriter.Write(data[k]);
                                fwriter.Write(",");
                            }
                            fwriter.Write("\n");
                        }
                    }
                }

            }
        }//rxThreadFunc()
        public void dataProc()
        {
            byte[] outData = new byte[datalen];
            while (true)
            {
                if (mQ.Count > 1)
                {
                    outData=mQ.Take();
                    using(StreamWriter fwriter=new StreamWriter("C:\\testsave\\dataProc",true))
                    {
                        for (int i = 0; i < datalen; i++)
                        {
                            fwriter.Write(outData[i]);
                            fwriter.Write(",");
                        }
                        fwriter.Write("\n");
                    }
                }
            }
        }

    }
}

Description: I write this application which contains two thread. RxThread receives data from serial port, sort it by header mark 0x55 0xaa, put the following 30 bytes into a FrameStruct class, then put this FrameStruct into Queue. dataProcess thread get frame from Queue, then store it to disk.

SerialPort=(rxbuff)=>RxThread=(rxFrame,Queue)=>dataProcess==>disk

Problem: Data received and saved into disk by dataProcess thread is somehow corrupted.

Tried: This is what I have tried for your reference.

I tried BlockedCollection which is naturally thread-safe instead of Queue, it still doesn't work. So I guess this is not Queue's problem.
I tried to add another member, int cnt, in FrameStruct, it self-increases right before messageQ.Enqueue() in RxThread. Then dataProcess thread can get it correctly. So I thought it might be data[] has something wrong, but...
but I tried to put byte data[30] instead of FrameStruct into Queue, doesn't work.
Also I think time received by dataProcess is also correct.

5. If I put a Thread.sleep(20) after Monitor.Pulse() in RxThread, problem solved, but I don't understand why??? And what if I changed to another computer?

This is the code snapshot.

//declared:
//Queue<FrameStruct>messageQ=new Queue<FrameStruct>;
//object _LockerMQ=new object();
private void RxThread()
    {
        int bytestoread, i;
        bool f55 = false;//55 flag
        bool fs = false;//frame start flag
        int j=0;//data index in FrameStruct
        int m_lMaxFram=32;
        bytestoread = 0;
        FrameStruct rxFrame = new FrameStruct((int)m_lMaxFrame);
        while (true)
        {
            if (Serial_Port.IsOpen == true)
            {
                if ((bytestoread = Serial_Port.BytesToRead) > m_lMaxFrame*2)//get at least two frames
                {
                        rxbuff = new byte[bytestoread];  
                        Serial_Port.Read(rxbuff, 0, bytestoread);
                        for (i = 0; i < bytestoread; i++)
                        {
                            if (rxbuff[i] == 0x55)
                            {
                                f55 = true;
                                continue;
                            }
                            if (rxbuff[i] == 0xaa && f55)
                            {//frame header 0x55, 0xaa
                                //new frame start
                                fs = true;
                                f55 = false;
                                j = 0;//rxframe index;
                                rxFrame.time = DateTime.Now;//store the datetime when this thread gets this frame
                                continue;
                            }
                            if (fs && j < m_lMaxFrame - 2)
                            {//frame started but not ended
                                rxFrame.data[j] = rxbuff[i];
                                j++;
                            }
                            if (j >= (m_lMaxFrame - 2) && fs)
                            {//frame ended if j=30, reaches the end of rxFrame.data
                                fs = false;
                                lock(_LockerMQ)
                                {
                                    messageQ.Enqueue(rxFrame);
                                Monitor.Pulse(_LockerMQ);
                                }
                                 //Thread.Sleep(20);//if uncomment this sleep, problem solved
                              using (StreamWriter fWriter = new StreamWriter("c:\\testsave\\RXdata", true))//save rxThread result into a file rawdata
                    {
                        fWriter.Write(rxFrame.time.ToString("yyyy/MM/dd HH:mm:ss.fff"));
                        fWriter.Write(",");
                        for (int k = 0; k < m_lMaxFrame - 2; k++)
                        {
                            fWriter.Write(rxFrame.data[k]);
                            fWriter.Write(",");
                        }
                        fWriter.Write("\n");
                    }
                            }
                           }
                }//if ((bytestoread=Serial_Port.BytesToRead) > 0)
                rxbuff = null;
                Thread.Sleep(20);
            }//(Serial_Port.IsOpen==true)
            Thread.Sleep(100);
        }//while(true),RxThread sleep
    }//private void RxThread()

DataProcess thread:

 public void dataProcess()
    {
      while (true)
        {
            lock (_LockerMQ)
            {
                while (messageQ.Count < 1) Monitor.Wait(_LockerMQ);//get at least one frame data
                f_NewFrame = messageQ.Count;
                if (f_NewFrame > 0)
                {
                    procFrame = messageQ.Dequeue();
                    using (StreamWriter fWriter = new StreamWriter("c:\\testsave\\dPdata", true))
                    {
                        fWriter.Write(procFrame.time.ToString("yyyy/MM/dd HH:mm:ss.fff"));
                        fWriter.Write(",");
                        for (int i = 0; i < m_lMaxFrame - 2; i++)
                        {
                            fWriter.Write(procFrame.data[i]);
                            fWriter.Write(",");
                        }
                        fWriter.Write("\n");
                    }
                }//if(f_NewFrame>0)

            }//lock(messageQ)
  }
}

FrameStruct contains members of time, and data[30]

class FrameStruct
{
        public FrameStruct(int m_lMaxFrame)
        {
            time = DateTime.Now;
            data = new byte[m_lMaxFrame - 2];
        }
        public DateTime time;
        public volatile byte[] data;//volatile doesn't help
}

rxData saved by RxThread is correct, shows:

2015/07/18 18:40:26.125,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,111,51,204,
2015/07/18 18:40:26.177,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,112,51,204,
2015/07/18 18:40:26.177,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,113,51,204,
2015/07/18 18:40:26.297,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,114,51,204,
2015/07/18 18:40:26.298,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,115,51,204,
2015/07/18 18:40:26.298,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,116,51,204,
2015/07/18 18:40:26.299,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,117,51,204,
2015/07/18 18:40:26.420,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,118,51,204,
                                                                                     //^this columns is accumulated number

dPdata saved by dataProcessThread is WRONG, shows:

2015/07/18 18:40:31.904,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,227,51,204,
2015/07/18 18:40:31.905,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,228,51,204,
2015/07/18 18:40:31.905,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,229,51,204,
2015/07/18 18:40:32.026,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,231,51,204,
2015/07/18 18:40:32.026,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,231,51,204,
2015/07/18 18:40:32.147,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,232,51,204,
2015/07/18 18:40:32.148,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,233,51,204,
2015/07/18 18:40:32.148,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,234,51,204,
2015/07/18 18:40:32.269,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,236,51,204,
2015/07/18 18:40:32.269,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,236,51,204,
2015/07/18 18:40:32.510,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,237,51,204,
2015/07/18 18:40:32.512,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,240,51,204,
2015/07/18 18:40:32.512,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,240,51,204,
2015/07/18 18:40:32.514,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,240,51,204,
2015/07/18 18:40:32.514,127,255,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,241,51,204,
2015/07/18 18:40:32.635,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,243,51,204,
2015/07/18 18:40:32.635,128,0,255,255,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,243,51,204,
                                                                                   //^this accumulated number is not correct

Please help!

Thank you!

Did you verify that the problem is multithreading? Did you test each component separately? — shay__, Jul 18 '15 at 11:46
Yes I tested it separately. I saved rawdata in rxThread, it shows OK. But rawdata1 in dataProcess is not OK. — aileenfun, Jul 18 '15 at 11:58
This guy might have similar problem, but his question is unanswered.[link](http://stackoverflow.com/questions/4537447/very-strange-and-severe-multithread-inconsistency-problem-c-sharp) — aileenfun, Jul 18 '15 at 12:08
Just trying to figure out what you try to do: do you want the writer to do his work as soon as you have one element in the queue? or you can allow the queue having more than one element? — Rami Yampolsky, Jul 18 '15 at 13:48
Here, in Update1 we have kind of bussy wait in the consumer. I see you dicided using BlockingCollection, this can make the consumer very simple. let me show you, I will add update — Rami Yampolsky, Jul 18 '15 at 15:44

score 0 · Answer 1 · answered Jul 18 '15 at 13:28

0

FrameStruct is a class (rather than a struct), and you are using the same reference over and over again when you enqueue it.

answered Jul 18 '15 at 13:28

shay__

3,815
17
34

Thanks mate, yet I tried to use byte[] frame instead of FrameStruct for queue, (Queue messageQ=new Queue, and messageQ.Enqueue(frame)),it doesn't work. Actually At first I declare FrameStruct as Struct, but I saw some people say it's better to have class than struct in C#, so I change it to class. – aileenfun Jul 18 '15 at 14:19
It doesn't matter,FrameStruct is a reference type then you have to initialize a new instance in every iteration. By the time your consumer picks an item off the queue, the producer modify the same item's data (if the next iteration executes in parallel with the consumer). I might be wrong here but you should double check that. – shay__ Jul 18 '15 at 18:20
yes you are right, I solved it. Thank you very much! – aileenfun Jul 19 '15 at 01:47
I was expecting you to accept that as an answer, but the important thing is that everything worked out for you :D – shay__ Jul 19 '15 at 05:44

Rami Yampolsky · Accepted Answer · 2015-07-18T18:10:40.123

Update 2

Here is basic producer consumer, just add your logic:

class Program
{

    static BlockingCollection<int> mQ = new BlockingCollection<int>();

    static void Main(string[] args)
    {

        Thread rxThread = new Thread(rxThreadFunc);
        rxThread.Priority = ThreadPriority.Highest;//this causes problem
        rxThread.Start();
        Thread procThread = new Thread(dataProc);
        procThread.Start();
        Console.ReadLine();
    }


    static public void rxThreadFunc()
    {
        for (int i = 0; i < 10; i++)
        {
            mQ.Add(i);
        }
    }


    static public void dataProc()
    {
        foreach (int outData in mQ.GetConsumingEnumerable())
        {
            Console.WriteLine(outData);
        }
    }


}

Answer for Update 1:

Yes this is the in the right place

Now, thanks to the BlockingCollection that you are using, the consumer (dataProc) may be very simple (just remove the loop and count checking, it would do all this syncronization for you):

 foreach (byte[] outData in _taskQ.GetConsumingEnumerable())
 {
     using(StreamWriter fwriter=new StreamWriter("C:\\testsave\\dataProc",true))
                {
                    for (int i = 0; i < datalen; i++)
                    {
                        fwriter.Write(outData[i]);
                        fwriter.Write(",");
                    }
                    fwriter.Write("\n");
                }
 }

Now, the problem may be different than in the original post. Maybe because here the producer writes the data to the file as well?

Original:

Its alot of explanations, but the solution is simple just add the initialization of FrameStruct to your second "if" statement:

rxFrame = new FrameStruct((int)m_lMaxFrame);

As shown in your saved data file: in the corrupted file, each time when you have missing value (for example 230) - you have other value duplicated (for example 231). The count of the missing values equals to the count of the duplicated values.

The reason for that is that you add reference to the same object instance to your queue. Let look on the following scenario: RxThread loops N times, before it context switched to the dataProcess thread, it adds N refferences to the same instance of FrameStruct to the queue. The data in this instance would be the data from the last read loop iteration before context switch. Now the context switch happens: the dataProcess loops M < N times before it context switched back to the RxThread, so it reads M elements from the queue, but all of them pointing to the same instance, so it writes M times same line to the file (the last one as explained before)

Now, why does the Thread.Sleep helps. The short answer: it makes a very hight probability to context switch to the dataProcess thread, each time that 1 element added to the queue by RxThread. So its actually: read one --> context switch --> write one... and same thing again.

The long answer would be: After dataProcess thread doing Monitor.Wait it goes to the waiting queue and context switch schedules the RxThread. Now the thread adds the first element to the queue and do Monitor.Pulse. This moves the dataProcess thread to the ready queue. But not necessarily schedules it for run immediately, so RxThread can make another iteration. But if you do the Thread.Sleep - it is a very hight probability that there will be context switch and ataProcess thread be scheduled now.

Thank you my friend. I think I understand your explaination, Please see **Update1**, In rxThreadFunc(), while(true){byte[]data=new byte[datalen];....} did I put it in the right place? it still doesn't work. — aileenfun, Jul 18 '15 at 15:13
I wrote Update1 code according to the same logic as my application, they share the same wrongs and rights while I test them. so I put byte[]data=new byte[datalen] in while(true)loop like you said(No "if" statement) — aileenfun, Jul 18 '15 at 15:19
Update 1 is a simplfied version of original application, I think they share the same problem, in Update1 I use Queue(byte[]) where byte[] is also a reference. But I put data=new bytes[datalen] in while(true) loop like you said, it still doesn't work. you can just copy this code into a new c# WinForm application. Both Update1's producer and original application's producer writes data into file, for debug purpose. — aileenfun, Jul 18 '15 at 16:26
No problems with syncronization in this code, I will paste you another update for "minical prducer consumer case", just add your logic to it. — Rami Yampolsky, Jul 18 '15 at 18:08
Thank you my friend, you are right about "put reference into queue". While I must put byte [] data=new data[len] (or rxFrame=new FrameStruct())in the place right after enqueue, it can make sure next frame get a fresh new data/rxFrame in next iteration. — aileenfun, Jul 19 '15 at 01:51
I have another question, will the compiler's garbage cleaner auto recycle former frame? Say rxFrame=frame1, enqueue(rxFrame), then in next iteration rxFrame=frame2, frame1 has no pointer to it anymore. Will cleaner think frame1 is useless and clean it? — aileenfun, Jul 19 '15 at 01:55
If object dont have ANY refference, then it is candidate to be cleaned. But in your case the object has refference by the queue, after that by the dataProcess method. So don't be worry about that, its always cleaned when you can't use it only. — Rami Yampolsky, Jul 19 '15 at 06:06

producer/consumer get different data by Queue

2 Answers2