Windows 7 ThreadpoolWait API design flaw?

[doug65536]

I have a project that uses DirectSound to play streaming sound.
I use SetThreadpoolWait to get a threadpool callback every time the notification (AutoReset) events are signalled. (To fill the streaming buffer with more audio).

The new Win7 threadpool API requires you to use SetThreadpoolWait to schedule a new wait every time it issues the callback - you need to reschedule a new wait every time it fires a callback.

Here is the class that I wrote to wrap the threadpool wait:

template<>
class ThreadpoolWaitImpl<true>
{
	AutoCloseThreadpoolWait wait;
	void *callbackTarget;
	HANDLE handle;
	FILETIME timeout;
	bool onlyOnce;
	bool shuttingDown;

public:
	typedef ThreadpoolWaitImpl<true> Wait;

	ThreadpoolWaitImpl()
		: callbackTarget(0)
		, handle(0)
		, onlyOnce(false)
		, shuttingDown(false)
	{
	}

	~ThreadpoolWaitImpl()
	{
		shuttingDown = true;
	}

	template<typename T, void (T::*func)(const Wait &, BOOLEAN)>
	BOOL SetWaitCallback(T *target, HANDLE waitObject, 
			ULONG milliseconds = INFINITE,
			ULONG flags = WT_EXECUTEDEFAULT)
	{
		callbackTarget = target;
		handle = waitObject;
		onlyOnce = !!(flags & WT_EXECUTEONLYONCE);

		wait = CreateThreadpoolWait(
				ThreadpoolWaitImpl<true>::TimerCallback_C<T, func>,
				this, NULL);
		if (!wait.IsValid())
			return FALSE;

		if (milliseconds == INFINITE)
		{
			SetThreadpoolWait(wait, waitObject, NULL);
		}
		else
		{
			LARGE_INTEGER timeoutUnion;
			timeoutUnion.QuadPart = milliseconds * 10000000;
			timeout.dwLowDateTime = timeoutUnion.LowPart;
			timeout.dwHighDateTime = timeoutUnion.HighPart;
			SetThreadpoolWait(wait, waitObject, &timeout);
		}
		return TRUE;
	}

	template<typename T, void (T::*func)(const Wait &, BOOLEAN)>
	static VOID CALLBACK TimerCallback_C(
			__inout     PTP_CALLBACK_INSTANCE instance,
			__inout_opt PVOID                 context,
			__inout     PTP_WAIT              wait,
			__in        TP_WAIT_RESULT        waitResult)
	{
		auto self = reinterpret_cast<Wait*>(context);
		auto target = reinterpret_cast<T*>(self->callbackTarget);
		(target->*func)(*self, waitResult == WAIT_OBJECT_0);
		if (!self->onlyOnce && !self->shuttingDown)
			SetThreadpoolWait(self->wait, self->handle, &self->timeout);
	}
};

AutoCloseThreadpoolWait is a RAII object that automates safely shutting down the threadpool object.

When AutoCloseThreadPoolWait is destructed it does the following:

class AutoDestructDestructPolicyCloseThreadpoolWaitSafe
{
public:
	static BOOL Destruct(PTP_WAIT wait)
	{
		SetThreadpoolWait(wait, NULL, NULL);
		WaitForThreadpoolWaitCallbacks(wait, TRUE);
		CloseThreadpoolWait(wait);
		return TRUE;
	}
};

There is a race condition here:

1) The threadpool wait fires and a threadpool thread calls TimerCallback_C . I'll call this thread CPU2. Imagine that it executed up to the point where it reads shuttingDown and it was false.
2) CPU1 is destructing the ThreadPoolWait object, which destructs the AutoCloseThreadpoolWait object, which invokes AutoDestructDestructPolicyCloseThreadpoolWaitSafe::Destruct , which invokes SetThreadpoolWait(wait, NULL, NULL).
3) (nanoseconds later) CPU2 executes SetThreadpoolWait(self->wait, self->handle, &self->timeout); to reschedule the callback.

Isn't it a huge design flaw in the new (win7) threadpool API to have to reschedule the wait every time? It creates the race condition with another thread that is attempting to shutdown the threadpool waits.

Anybody know a reliable way to stop a threadpool wait and clean it up reliably?

[doug65536]

As a workaround, I am using a slim reader writer lock and doing a TryAcquireSRWLockShared. If it acquires the lock, I proceed to test the shutdown flag etc. Before shutting down, I acquire exclusive lock and set the shuttingDown flag. If the threadpool callback is racing the shared acquire will lockout the shutdown code until the callback returns. If the shutdown gets the exclusive lock first, the racing thread will fail to TryAcquireSRWLockShared.

Any ideas how to avoid the SRW lock though?

[vijayan121]

Use a Threadpool Cleanup Group to manage graceful shutdown.

http://msdn.microsoft.com/En-US/library/ms682036%28v=VS.85%29.aspx

[doug65536]

As far as I can tell from the documentation, doing WaitForThreadpoolWaitCallbacks will do the same thing as the cleanup group (the cleanup group is also a "container" for several threadpool objects - tied together by the threadpoolenvironment, automating waiting for all the callbacks, etc, and closing them all).

The documentation even mentions an exception is possible if you set a wait concurrently with the cleanup. That is what is going to happen, because the wait callback has to setup the next wait every time, it's going to be setting a new wait every time it fires to get the next waited-for event.

The issue is, because the new wait must be continually reissued every time the wait fires (from the wait callback), it creates a race between code that is trying to stop the wait callbacks and the code in the callback that is trying to setup the next wait.

[Klaus_1]

You are right that the docs are very unclear about this. I think however that calling SetThreadpoolWait isn't necessary for cleanup, so destruction can be simplified:

static BOOL Destruct(PTP_WAIT wait)
{
    WaitForThreadpoolWaitCallbacks(wait, TRUE);
    CloseThreadpoolWait(wait);
    return TRUE;
}

My understanding is that there is no race condition between thread CPU2 and CPU1 because WaitForThreadpoolWaitCallbacks waits until CPU2 has completed its TimerCallback_C callback, regardless of whether it meanwhile reregistered another wait.

I presume the WaitForThreadpoolWaitCallbacks(wait, TRUE), CloseThreadpoolWait(wait) sequence is the same as using the legacy threadpool API UnregisterWaitEx(wait, INVALID_HANDLE_VALUE)