How Multithreading in c++ is done

[venkat raghvan]

I have design one c++ multithreading application (first time) in which one thread does following task: it read values from file & insert into dynamic queue. Second thread does read values from queue and write into output file; Sometimes task is done as i wish like 'Queue is write complete'(output of first thread) and after 'Queue read completed'(output of second thread),but sometimes it behaves abnormal means it shows output of second thread & then output of first thread;also sometimes output of first thread,not second and wiseversa.
Below i copied my code,please suggest any changes required for it.
Also my doubt:Is that operating system monitors these threads,so it shows different output?

#include <iostream>
using namespace std;
#include<windows.h>
const int MAX = 2*1048576;
int Size ;
HANDLE hThread1,hThread2;
DWORD dwID1,dwID2;
DWORD Thread1(LPVOID param);
DWORD Thread2(LPVOID param);
FILE *fp,*fpOut;
  class cqueue
  {
	 
	public :
		static int front,rear;
		static int *a;//=new int[1048576];
	   cqueue()
	   {
		 front=rear=-1;
		 a=new int[MAX];
	   }
	   ~cqueue()
	   {
		delete[] a;
	   }
	  
	  static void insertWithAck(char);
	  static int deletionWithAck();
  };
 
int cqueue::front;
int cqueue::rear;
int* cqueue::a;
 
void cqueue :: insertWithAck(char val)
{
 while((rear==MAX-1) || (rear+1==front)) ;
   if(rear==MAX-1)
	  rear=0;
   else	
	 rear++;
   a[rear]=val;
 if(front==-1)
   front=0;
}
 
int cqueue :: deletionWithAck()
{
 char k;
 while(front== -1);
	
	k=a[front];
	if(front==rear)
	   front=rear=-1;
	else
	{
	   if(front==MAX-1)
		  front=0;
	   else
		  front++;
	}
 
 return k;
}
DWORD Thread1(LPVOID param)
{
	int i,val;
 
	fp=fopen("1MB_TEST_FILE.txt","rb");
	fseek(fp,0,SEEK_END);
	Size=ftell(fp);
	rewind(fp);
	for(i=0;i<Size;i++){
			fread(&val,1,1,fp);
			cqueue::insertWithAck(val);
	}
	fclose(fp);
	cout<<"Queue write completed\n";   
   return 0;
}
DWORD Thread2(LPVOID param)
{	
	fpOut=fopen("OutFile.txt","wb");
	char retVal;
	
	for(int i=0;i<Size;i++){
		retVal=cqueue::deletionWithAck();
		fwrite(&retVal,1,1,fpOut);
	}
	fclose(fpOut);
	cout<<"Queue read completed\n";
	return 0;
}
 
void main()
{
	 cqueue c1;
	 hThread1= CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)Thread1,NULL,0,&dwID1);
	
	 hThread2= CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)Thread2,NULL,0,&dwID2);
system("pause")
}

[kbw]

static members/data in your class? I think I'll be bold and and say that's wrong. I just can't imagine a scenario where that's helpful.

In Windows, never use CreateThread in a C/C++ program. You need to use _beginthreadex instead.
http://msdn.microsoft.com/en-us/library/kdzttdcb.aspx

main() needs to wait for the threads to complete with WaitForMultipleObjects.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms687025%28v=vs.85%29.aspx

There's no synchronisation between threads. You will have race conditions.

[andywestken]

Your stack class has methods whose names include "Ack" but I see no acknowledgement (or notification) code. How did you expect the threads to work together??

Andy

PS In addition to kbw's comments:

- you have too may globals, inc. the stack (effectively, as all it members are static -- the variable on line 97 is actually unused.)

- the (LPTHREAD_START_ROUTINE) casts should be removed as it masks a problem with function calling convention. Your thread functions should be declared with a specific calling convention: WINAPI

DWORD WINAPI Thread1(LPVOID param)
{
    // etc 

Sames holds for _beginthreadex, except the MSDN entry states the calling convention of the thread function should be __stdcall

(And unless you are absolutely sure you will be using none of the CRT functions, you should be calling _beginthreadex rather than CreateThread.)

[Computergeek01]

Can I ask anyone to elaborate on what is wrong with using the "CreateThread()" function? I feel like I missed a memo somewhere and the only search results I read are people complaining about the function because they don't know how to use calling conventions.

EDIT: I think I found something. Is this about CreateThread() not initializing MS CRT? Because you're not supposed to mix and match WinAPI and MSCRT for half a dozen other reasons anyway.

EDIT facepalm: From thread.c in the Microsoft Platform SDK. See Line 38 posted here or Line 96 from the source if you have it. I feel dumb for asking since I've had this code for a few years now.

uintptr_t __cdecl _beginthread (
        void (__cdecl * initialcode) (void *),
        unsigned stacksize,
        void * argument
        )
{
        _ptiddata ptd;                  /* pointer to per-thread data */
        uintptr_t thdl;                 /* thread handle */
        unsigned long errcode = 0L;     /* Return from GetLastError() */

        if ( initialcode == NULL ) {
            errno = EINVAL;
            return( (uintptr_t)(-1) );
        }

        /*
         * Allocate and initialize a per-thread data structure for the to-
         * be-created thread.
         */
        if ( (ptd = _calloc_crt(1, sizeof(struct _tiddata))) == NULL )
                goto error_return;

        /*
         * Initialize the per-thread data
         */

        _initptd(ptd);

        ptd->_initaddr = (void *) initialcode;
        ptd->_initarg = argument;

        /*
         * Create the new thread. Bring it up in a suspended state so that
         * the _thandle and _tid fields are filled in before execution
         * starts.
         */
        if ( (ptd->_thandle = thdl = (uintptr_t)
              CreateThread( NULL,
                            stacksize,
                            _threadstart,
                            (LPVOID)ptd,
                            CREATE_SUSPENDED,
                            (LPDWORD)&(ptd->_tid) ))
             == (uintptr_t)0 )
        {
                errcode = GetLastError();
                goto error_return;
        }

        /*
         * Start the new thread executing
         */
        if ( ResumeThread( (HANDLE)thdl ) == (DWORD)(-1) ) {
                errcode = GetLastError();
                goto error_return;
        }

        /*
         * Good return
         */
        return(thdl);

        /*
         * Error return
         */
error_return:
        /*
         * Either ptd is NULL, or it points to the no-longer-necessary block
         * calloc-ed for the _tiddata struct which should now be freed up.
         */
        _free_crt(ptd);

        /*
         * Map the error, if necessary.
         */
        if ( errcode != 0L )
                _dosmaperr(errcode);

        return( (uintptr_t)(-1) );
}

[kbw]

All those per thread statics in the C runtime library (like errno, the return from localtime, the buffer in strtok, ...) don't get set up if you don't call _beginthreadex(). Forget _beginthread().

[venkat raghvan]

how to use synchronization in this code?that means it shows expected output every time.

[kbw]

As you're just synchronising threads, you should use a CriticalSection.

The basic problem is that as threads run asynchronously and concurrently, access to shared data must be synchronised. For example, you can't read while something else is writing as you may get the old, new value, or some intermidate state.

You can learn more about here, and checkout out the links at the end of the page.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682530%28v=vs.85%29.aspx

[andywestken]

To signal that you've added data, you could use an event (along with the CriticalSection to serialize access.) The writer thread (Thread2) would need to wait for this event to be signalled before extracting data.

The same event, with suitable flags, could be used to signal the thread to exit.

Andy

PS The code you posted has other problems: the while loops on lines 37 and 50 don't look quite right.

PPS As your coding in C++ you might want to make use of ifstream/ifstream (rather than fopen, etc) and std::queue