views:

1638

answers:

7

I have got myself stuck into a really amazing issue here.The code is like as below.

class A
{
   public:  
   A(){  
         m_event =  CreateEvent(NULL, false, false, NULL);       // create an event with initial value as non-signalled
         m_thread = _beginthread(StaticThreadEntry, 0, this);    // create a thread 
      }

   static void StaticThreadEntry(A *  obj) {  obj->ThreadEntry();  }

   void ThreadEntry();
};

void A::ThreadEntry()
{
     WaitforSingleObject(m_event,INFINITE);
}


int main()
{
        A a;

        SetEvent(m_event);     // sets the event to signalled state which causes the running thread to terminate 

        WaitForSingleObject(m_thread, INFINITE);  // waits for the thread to terminate

        return 0;
 }

Problem :

When the above code is run,sometimes( 1 out of 5 ) it hangs and the control gets stuck in the call to WaitforSingleObject()( inside the main function ).The code always calls SetEvent() function to ensure that the thread would terminate before calling Wait() function.

I do not see any reason why it should ever hang?

+2  A: 

I don't see any issues in the code (Assuming event is created before the thread is started in constructor).

  • The event is a auto reset event and the initial state is non signaled.
  • Child thread has to wait until the event is signaled.
  • Main thread will signal the event and waits for child process to terminate.

Assuming this is the complete code (not the sample code ), it looks quite fine to me.

I suggest you to use process Explorer to observe the state of the event.

EDIT:

There is a slight chance that child thread gets terminated before the main thread waits on the thread handle. If the handle is reused for some other kernel objects and main thread will wait infinitely. Try duplicating the handle using DuplicateHandle after thread creation and use this handle in WaitForSingleObject.

aJ
Yes the event is constructed before the thread is created. Yeah,as you stated this code is fine.Answers above are just not making any sense.
Rakesh Agarwal
Can you edit the code in question to reflect the order of event creation?
aJ
Handle duplication is quite possible as Windows try to reuse handle values.
Canopus
Ok,I will look into this.
Rakesh Agarwal
Do not use _beginthread, use _beginthreadex instead. This way you will be responsible for closing the handle. (You need to close the handle then using CloseHandle once you have waited for it)
Suma
yeah I am trying _beginthreadx() and _endthreadx() now.
Rakesh Agarwal
If the handle is closed too quickly to call WaitForSingleObject, then it's also closed too quickly to call DuplicateHandle. Apparently, the return value of _beginthread can be obsolete as soon as the function returns. (Why was it ever written that way?)
Rob Kennedy
@Canopus Are you sure that handles can be re-used? HWNDs aren't (very likely to be) re-used in NT or better (http://blogs.msdn.com/b/oldnewthing/archive/2007/07/17/3903614.aspx) and I would imagine Windows uses a similar scheme for any handles. Ditto for WinCE 6 http://msdn.microsoft.com/en-us/library/ee482861.aspx. With a 16-bit counter your odds of getting a handle that used to exist are probably low enough not to worry about.
mhenry1384
Yes it does reuse the handle values. Why not right a small test program to verify it? HANDLE hEvent1 = CreateEvent(NULL,FALSE,FALSE,NULL); CloseHandle(hEvent1); HANDLE hEvent2 = CreateMutex(NULL, TRUE, NULL); if (hEvent1 == hEvent2) { std::cout << "Handle Values are same"; }
Canopus
+1  A: 

you might want to consider using SignalObjectAndWait instead of the seperate SetEvent() and WaitForSingleObject() calls, as that is an atomic operation, and would fail immediatly if the event could not be signaled.

Hasturkun
Yeah, I could try this.Thanks.
Rakesh Agarwal
+1  A: 

Have you checked whether the thread handle m_thread is actually valid?

There are circumstances where _beginthread will return an invalid handle - particularly when the thread exits quickly (which certainly could be the case here as the thread could spin up, pass through the wait (as the event is already set) and then terminate).

Use _beginthreadex instead to create the handle, although you would have to call _endthreadex to make sure everything was cleaned up.

Alan Moore
The return value of _beginthread() is ok.I checked that.
Rakesh Agarwal
To be clear, in the case Alan Moore is noting, _beginthread()'s return value is a *valid* HANDLE, just no longer a HANDLE for this particular thread, or possibly not even a thread at all. If it happens to be a HANDLE that will never be signaled, (such as a typical file HANDLE) then the wait can never be satisfied.
RBerteig
+6  A: 

The problem is your use of the _beginthread API. You cannot use the handle returned from this function with the Win32 wait functions. You should use _beginthreadex or CreateThread. From MSDN:

If successful, each of these functions returns a handle to the newly created thread; however, if the newly created thread exits too quickly, _beginthread might not return a valid handle...

You are ... able to use the thread handle returned by _beginthreadex with the synchronization APIs, which you cannot do with _beginthread.

Peter Ruderman
Are you sure about this fact : You cannot use the handle returned from this function with the Win32 wait functions
Rakesh Agarwal
Yeah, I just checked that in the MSDN website.Its strange- I didnt know that.
Rakesh Agarwal
beginthreadex() seems to be working - for now it is not hanging at all
Rakesh Agarwal
A: 

The thread will hang if SetEvent executes before WaitforSingleObject in the thread. So in that case WaitforSingleObject(m_event,INFINITE) will wait forever.

Test
A: 

Is it not bad practice to hand out the address of an object prior to that object being fully constructed. We have control of when the new thread will execute obj->ThreadEntry(), it may be after the constructor has completed, there is a full object on which to call ThreadEntry, or ThreadEntry may begin prior to the object being constructed.

martsbradley
A: 

Hasturkun says that SignalObjectAndWait is an atomic operation.

However, MSDN disagrees:

Note that the "signal" and "wait" are not guaranteed to be performed as an atomic operation. Threads executing on other processors can observe the signaled state of the first object before the thread calling SignalObjectAndWait begins its wait on the second object.

Ya, I'm a little confused myself and my head hurts right now, but I'm sure I'll figure it out. It may not be atomic, but it appears to say that it accomplishes making sure that your thread is waiting on the object before the object to be signaled is signaled. Right?

MarkS