threads (B)

[sky_00]

here are two threads that are to work on incrementing a value and outputting it using semaphores. is this best method considering speed?.
DestroyWindow() is used to make sure threads are exited.

semaphore = new CSemaphore(2, 2);

static int incr = 0;
static bool output = false;
static bool stop = false;

void CRunThreadDlg::Go()
{
incr = 0;
output = false;
stop = false;
HWND hWnd = GetSafeHwnd();
AfxBeginThread(ThreadProc1, hWnd);
AfxBeginThread(ThreadProc2, hWnd);
}

UINT ThreadProc1(LPVOID param)
{
CSingleLock singleLock(semaphore);
singleLock.Lock();
while(!stop)
{
if(!output)
{
incr++;
output = true;
}
}
return 0;
}
UINT ThreadProc2(LPVOID param)
{
CSingleLock singleLock(semaphore);
singleLock.Lock();
while(!stop)
{
if(output)
{
cout(incr);
output = false;
}
}
return 0;
}


void CRunThreadDlg::Onstop()
{
stop = true;
}


BOOL CRunThreadDlg::DestroyWindow()
{
stop = true;
delete semaphore;
Sleep(2000);
return CDialog::DestroyWindow();
}

[riteshtandon]

I cant say what you are trying to achieve with this code. but the semaphore is useless here as once you enter the loop then you run into it forever till stop becomes true. so there is no effect of semaphore usage.

[Andreas Masur]

[ Redirected thread ]

[Andreas Masur]

Despite any speed considerations, the code does not work as you would expect it. The thread that first owns the semaphore will run until the 'stop' variable will be triggered while the second thread will wait for the semaphore forever.

Besides that....if you just want to increment an integer, you might be better off using functions such as 'InterlockedIncrement()' (http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/interlockedincrement.asp)...

[upashu2]

If you want to only increment/decrement the counter , then You may use InterLockedIncrement()/InterLockedDecrement(). Again in your code, you are sharing bool variables (stop and output) without any thread-safety.
BOOL CRunThreadDlg::DestroyWindow()
{
stop = true;
delete semaphore;
Sleep(2000);
return CDialog::DestroyWindow();
}
Here you are setting stop = true,and immediatly
calling delete semaphore. Just setting stop = true it doesn't means that all thread will be terminated at this point. And when they tried to access on a deleted semaphore, anything ( e.g.Access violation) may occur.
Again providing sleep is not guaranttee for sychronization.Also see post#4.

[upashu2]

See following link for when to use semaphore
http://www.codeproject.com/threads/semaphores.asp
http://www.codeproject.com/threads/Synchronization.asp
http://www.spinwardstars.com/frontier-tutorials/threads/page04safety.html
http://www.codeproject.com/debug/cdbntsd7.asp

[MikeAThon]

I don't think your program will do what you expect it to do. You constructed your CSemaphore object with a maximum access count of "2", which means that both of your threads will be able to acquire the semaphore, and neither thread will be blocked by the other. Thus, you are not providing exclusive (i.e., one thread at a time) access to the "incr" variable, and both threads will be able to access it at the same time.

Of course, in the CSemaphore constructor, you also specidied an initial access count of "2", so maybe both of your threads are blocking on their respective Lock() functions???

Finally, in the example you have shown us, there is really no need for exclusive access to the "incr" variable, since it's POD and since only one thread is writing its value and all other threads are merely reading the value.

The variable that needs exclusion for its access is the "output" variable, since both threads are writing to it. Protect the writes by bracketing the "output=..." statements in each thread with calls to Lock/Unlock (i.e., your Lock/Unlock calls should protect the smallest amount of code possible, and should not bracket the entire "while" loop).

If you do this, please note that the expected output will not include each incremented value of incr. Rather, you will see "snapshots" of the incremented value, possibly with very large differences in value between each "cout"


Mike

[Andreas Masur]

Of course, in the CSemaphore constructor, you also specidied an initial access count of "2", so maybe both of your threads are blocking on their respective Lock() functions???
Didn't even noticed that....in this case, you are right...both threads should block...

Finally, in the example you have shown us, there is really no need for exclusive access to the "incr" variable, since it's POD and since only one thread is writing its value and all other threads are merely reading the value.
Well...nevertheless, that assumes that incrementing the value is an atomic operation...

[MikeAThon]

As another good coding practice, all the the variables whose values might be changed by another thread should be declared as "volatile", primarily so that an optimizing compiler doesn't do things it shouldn't.

For example, look at the following code, which is excerpted from the OP:
static bool output = false;

// ...

UINT ThreadProc2(LPVOID param)
{
CSingleLock singleLock(semaphore);
singleLock.Lock();
while(!stop)
{
if(output)
{
cout(incr);
output = false;
}
}
return 0;
}


In this example, it's possible (though not very likely) that an optimizing compiler might notice that "output" is defined to a value of "false", and might therefore conclude that the "if" statement will never be executed. As a consequence, this same optimizing compiler might eliminate the "if" statement in its entirety.

Better coding practice would declare the values as "volatile". See http://msdn.microsoft.com/library/en-us/vclang/html/_langref_volatile.asp

Mike