By the way, thank you for the very thorough response.
I particularly enjoy the phrase "deep and fundamental problem". Since
Joseph M. Newcomer wrote:
The ;whole concept is more than a little scary. It seems a rather convoluted way to get a
timer event to happen; you would probably find it much simpler to use the multimedia
timers. The callback for a multimedia timer runs in a separate thread.
You could replace all of this code by
timeSetEvent(1000, resolution, timeproc, userdata, TIME_PERIODIC);
and you use timeKillEvent to kill off (abort) the timer. Given that two lines of code
replace the complexity below, I would urge you to use the simpler method.
See below...
On 10 Aug 2006 06:33:24 -0700, "PaulH" <paul.heil@gmail.com> wrote:
I have a class I use as a timer, CAbortableTimer(), that has, until
now, worked pretty well. But, in my current situation, I have a class
that starts a timer in its constructor and kills it in its destructor.
But, when the destructor is called sometimes I get an Access Violation.
****
I don't suppose you've looked at why. The call stack should give useful information as to
what went wrong.
It is entirely possible that the timer ticked in a race condition such that the callback
was invoked just as the destructor was being invoked. Note that the SetEvent does NOT
cause the thread to instantly activate; in a hyperthreaded processor or multiprocessor,
the thread could also be running and preparing to call the callback already. There are so
many fundamental synchronization errors possible here that the mind boggles.
****
In this case, CMyClass is allocated on the stack, so the destructor is
called by delete.
The callstack at the time of the Access Violation is, unfortunately,
junk. Can anybody tell from the code below where I might be going
wrong?
Thanks,
PaulH
CMyClass::CMyClass()
{
m_hTimer = CAbortableTimer::SetTimer(1000, &CMyClass::OnTimer,
(DWORD_PTR)this);
}
CMyClass::~CMyClass()
{
if (m_hTimer != INVALID_HANDLE_VALUE)
SetEvent(m_hTimer); //stop the timer
}
//The timer should fire every dwMilliseconds and call the timer
callback function.
//The timer can be killed by calling SetEvent on the HANDLE returned by
this function.
/*static*/ HANDLE CAbortableTimer::SetTimer(DWORD dwMilliseconds,
const LPATIMERCALLBACK
fptc,
DWORD_PTR dwUserData)
{
PTIMER_INFO pTI = new TIMER_INFO;
pTI->dwMilliseconds = dwMilliseconds;
pTI->fptc = fptc;
pTI->dwUserData = dwUserData;
pTI->hAbort = CreateEvent(NULL, TRUE, FALSE, _T("AbortEvent_" +
GUIDGen()));
*****
There is no need to name the timer; in fact, the name should be irrelevant because it is
being used within a single process. WIthout the name, CreateEvent will create a unique
even each time. This is a minor detail
*****
if (!pTI->hAbort)
{
delete pTI;
return INVALID_HANDLE_VALUE;
}
AfxBeginThread(TimerThread, reinterpret_cast<LPVOID>(pTI));
return pTI->hAbort;
*****
There are potential race conditions here. Either the worker thread has responsibility for
this, or this thread has responsibility, but when you split the responsibility there are
serious risks
*****
}
/*static*/ UINT CAbortableTimer::TimerThread(LPVOID lParam)
{
PTIMER_INFO pTI = reinterpret_cast<PTIMER_INFO>(lParam);
DWORD dwWait = 0;
do
{
dwWait = WaitForSingleObject(pTI->hAbort, pTI->dwMilliseconds);
if (dwWait == WAIT_TIMEOUT)
{
//timer finished
ASSERT_POINTER(pTI->fptc, LPATIMERCALLBACK);
// If the above ASSERT fires, it is likely the calling
class was
// destroyed before the timer was shut down. You should
shut down
// the timer in the destructor.
****
Actually, no. This just asserts that the value in pTI->fptc is still a valid pointer. It
might be, even if the object was destroyed. The value on the heap, even though it is in
free space now, might still remain valid.
Now, note that since this is in a separate thread, it could be preparing to do this
callback just as your destructor has run. While you indeed do the SetEvent which will
cause this to exit the next time around, it is ALREADY HERE at the point where the
SetEvent happens and the storage is freed, and THEN it tries to do this call.
This is a deep and fundamental synchronization problem. One horrible kludge that might
work is to change your destructor to do
if (m_hTimer != INVALID_HANDLE_VALUE)
{
SetEvent(m_hTimer); //stop the timer
::WaitForSingleObject(threadobject->m_hThread, INFINITE);
delete threadobject;
}
but that has its own problems. For example, you can't just AfxBeginThread; you will have
to AfxBeginThread(...CREATE_SUSPENDED...), then set the m_bAutoDelete flag to FALSE before
doing a ResumeThread; you will need to keep the thread handle around so you can wait on
it, etc.
Overall, as I indicated, this is a clumsy solution (and erroneous solution) to a much
simpler problem. Overall, trying to make sure the timer thread (this one or one being
used by timeSetEvent) will not conflict with the destruction of the MyClass object is a
nontrivial issue. I am not even certain my code above is going to be correct for this,
but it is far more correct than what is there. But note that it adds even more complexity
to an already overly-complex solution.
****
pTI->fptc(pTI->dwUserData);
****
You have not shown the OnTimer function. This would be useful information. The prototype
shown below only says the parameters match; it doesn't let us see what race conditions
might exist. We don't see where potential for concurrent access might exist which would
result in problems. Since that function is being called from a separate thread, any
accesses to shared variables being used by any other thread must be synchronized properly.
****
}
else if (dwWait == WAIT_OBJECT_0)
{
//timer aborted
}
} while(dwWait == WAIT_TIMEOUT);
*****
It would probably make more sense to while(true) this loop and have the "timer aborted"
condition do a "break". The code would be easier to read.
*****
delete pTI;
****
Who closes the event handle?
****
return 0;
}
typedef struct _timerInfo
{
DWORD dwMilliseconds;
HANDLE hAbort;
LPATIMERCALLBACK fptc;
DWORD_PTR dwUserData;
}TIMER_INFO, *PTIMER_INFO;
typedef void (CALLBACK ATIMERCALLBACK)(DWORD_PTR dwUserData);
typedef ATIMERCALLBACK FAR *LPATIMERCALLBACK;
Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm